Skip to content
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

Closed
MichaelChirico opened this issue Jul 18, 2023 · 5 comments
Closed

Avoid readability-avoid-const-params-in-decls? #1271

MichaelChirico opened this issue Jul 18, 2023 · 5 comments

Comments

@MichaelChirico
Copy link
Contributor

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 since const 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 whether const 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.

@eddelbuettel
Copy link
Member

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?

@MichaelChirico
Copy link
Contributor Author

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.

@eddelbuettel
Copy link
Member

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.

@MichaelChirico
Copy link
Contributor Author

:)

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.

@eddelbuettel
Copy link
Member

"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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants