Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Netron output (ONNX JSON) #3717
Netron output (ONNX JSON) #3717
Changes from 23 commits
fc41a6f
7f79ce2
0796820
0c01550
b49a762
0dd2876
74870fb
25e76a0
e7a40ab
0e16a1a
a375111
fe76f7e
552638b
1d89e41
755ce18
d2b16cc
936dba9
8bde3ef
18b373f
b3f99eb
f89dfd5
965644a
937836f
cf9271b
a195d07
a758744
7a2ee8d
211b2d7
18019ed
a062ef8
5e92ff9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 33 in src/base64.cpp
GitHub Actions / tidy
Check warning on line 35 in src/base64.cpp
GitHub Actions / tidy
Check warning on line 42 in src/base64.cpp
GitHub Actions / tidy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result should be stored as
std::string
, there is no reason to prefill it with=
, you justpush_back
the characters.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
push_back
onto strings of very large lengths can be a performance issue. Ideally the space should be just reserved, rather than filled with=
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reserve
can be called if there is a perf issue. I dont think its that important here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't use std::string as is because the type being
unsigned char
is important.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could probably use
basic_string<unsigned char>
, but that's more code changes for what looks like a marginal benefitThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use the iterators directly for the loop:
And add the
to_int
function to bit_cast it and convert it tostd::size_t
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it would be better to make a function to encode the triplet since you repeat the same below for the "padding":
Then the loop can do:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as iterator usage is concerned,
lt
orgt
comparison is not recommended againstbuf.end()
. It should strictly be used by an equality comparison:==
or!=
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not recommended, by who? These are random access iterators so they support comparison operators just like pointers. The
<
operator should definitely be used here since we are skipping over by increments of 3, which means it could skip past the end, and it would become an infinite loop since we would never reach the end as we already past it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it could be UB if its past
buf.end()
so we probably need to dobuf.end() - remaining
to avoid that.Check warning on line 56 in src/base64.cpp
GitHub Actions / tidy
Check warning on line 57 in src/base64.cpp
GitHub Actions / tidy
Check warning on line 58 in src/base64.cpp
GitHub Actions / tidy
Check warning on line 60 in src/base64.cpp
GitHub Actions / tidy
Check warning on line 62 in src/base64.cpp
GitHub Actions / tidy
Check warning on line 65 in src/base64.cpp
GitHub Actions / tidy
Check warning on line 65 in src/base64.cpp
GitHub Actions / tidy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can reuse the
encode
function so it doesnt repeat the code. You can also copy the chars into an array(this waypad_cond
is not getting sublty modified):Check warning on line 74 in src/base64.cpp
GitHub Actions / cppcheck