-
-
Notifications
You must be signed in to change notification settings - Fork 212
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
Avoid readability-avoid-const-params-in-decls? #1271
Comments
I am a little lost now. What do you (realistically) expect us to do there? Reimplement a llvm parser? Methinks the tool you want to use exists, the repo you want apply it too exists (and you reference it three times). But you file a ticket here? |
Rcpp is responsible for creating RcppExports.cpp, right? And in doing so it's copying the definition over to a declaration. That's where Rcpp comes in. |
Sure. Mechanically true. Rcpp also does not write the initial declaration. Anyway, your itch to scratch so I look forward to any PRs from you 😉. File is https://github.com/RcppCore/Rcpp/blob/master/src/attributes.cpp and a mere 3700 lines. |
:) filed on the off chance this fits easily into something that routine's already doing. it's definitely not worth investing much new effort into. |
"It's hard." The src/attributes.cpp code is good and clever and not too ambitious. C++ is a complicated languages. Sometimes doing less is better. |
Documentation:
https://clang.llvm.org/extra/clang-tidy/checks/readability/avoid-const-params-in-decls.html
Came across this in quanteda, e.g.
definition:
https://github.com/quanteda/quanteda/blob/db2466641c51c4c5de5a199d350550e3434946b4/src/tokens_select.cpp#L141-L149
declaration:
https://github.com/quanteda/quanteda/blob/db2466641c51c4c5de5a199d350550e3434946b4/src/RcppExports.cpp#L230
As noted in the docs, copying over such
const
qualifications is somewhat misleading sinceconst
doesn't do anything there. OTOH, there's a readability argument that having a 1-1 match of declaration+definition makes for easier maintenance.There's also nothing wrong, per se, with copying over the
const
, so it may be more pain that it's worth to add logic to figure out whetherconst
can be ignored in a given case (assuming no such logic is in place already).But, I came across this, so I figured I'd file the issue. Feel free to close as out of scope.
The text was updated successfully, but these errors were encountered: