[PATCH] D33531: Clang-tidy readability: avoid const value return
PiotrZSL abandoned this revision. PiotrZSL marked 4 inline comments as done. PiotrZSL added a comment. Obsolete, this check is already added. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D33531/new/ https://reviews.llvm.org/D33531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33531: Clang-tidy readability: avoid const value return
sbenza added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:31 + isAnyPointer(), + references(type()), + templateTypeParmType(), References are never const qualified, right? Comment at: test/clang-tidy/readability-const-value-return.cpp:51 +const T f_returns_template_param(); + +template We are missing one like: template T f(); where `T` is `const X`. We also need to test for macros (and skip fixes there) Repository: rL LLVM https://reviews.llvm.org/D33531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33531: Clang-tidy readability: avoid const value return
aaron.ballman added a comment. In https://reviews.llvm.org/D33531#767640, @alexfh wrote: > In https://reviews.llvm.org/D33531#767628, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D33531#767059, @alexfh wrote: > > > > > > Would it make sense to silence this diagnostic in the presence of also > > > > checking for cert-dcl21-cpp for such operators? > > > > > > Currently there's no mechanism in clang-tidy to express dependencies or > > > compatibility issues between checks. Moreover, we already have checks > > > that are not meant to be run together, for example, > > > readability-braces-around-statements and its google- incarnation (and > > > other alias checks with different settings). That said, we could > > > whitelist postfix increment and decrement operators in this check. > > > Camillo, WDYT? > > > > > > I can imagine a generic whitelist mechanism might be useful for this check. > > It could even be empty by default, but the documentation could call out > > cert-dcl21-cpp specifically and show an example of how you can run both > > checks. > > > A generic whitelist of method/function names would make sense, if we had more > use cases for it. It might also be quite tricky to implement: distinguishing > between prefix and postfix increment/decrement operators would require > specifying arguments, and allowing it for all types would need a support for > pattern matching or optional omission of the type name on methods. All this > seems to be an overkill so far. Good point on the prefix/postfix nature. This does seem like overkill. > If we want this whitelisting be optional, we can add a boolean option > specifically for these operators. In light of a more general solution, I say we don't add any configuration option. If it turns out people want to run both of these checks at the same time a lot in practice, we can address it with a more general mechanism to express dependencies/conflicts. Repository: rL LLVM https://reviews.llvm.org/D33531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33531: Clang-tidy readability: avoid const value return
alexfh added a comment. In https://reviews.llvm.org/D33531#767628, @aaron.ballman wrote: > In https://reviews.llvm.org/D33531#767059, @alexfh wrote: > > > > Would it make sense to silence this diagnostic in the presence of also > > > checking for cert-dcl21-cpp for such operators? > > > > Currently there's no mechanism in clang-tidy to express dependencies or > > compatibility issues between checks. Moreover, we already have checks that > > are not meant to be run together, for example, > > readability-braces-around-statements and its google- incarnation (and other > > alias checks with different settings). That said, we could whitelist > > postfix increment and decrement operators in this check. Camillo, WDYT? > > > I can imagine a generic whitelist mechanism might be useful for this check. > It could even be empty by default, but the documentation could call out > cert-dcl21-cpp specifically and show an example of how you can run both > checks. A generic whitelist of method/function names would make sense, if we had more use cases for it. It might also be quite tricky to implement: distinguishing between prefix and postfix increment/decrement operators would require specifying arguments, and allowing it for all types would need a support for pattern matching or optional omission of the type name on methods. All this seems to be an overkill so far. If we want this whitelisting be optional, we can add a boolean option specifically for these operators. Repository: rL LLVM https://reviews.llvm.org/D33531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33531: Clang-tidy readability: avoid const value return
aaron.ballman added a comment. In https://reviews.llvm.org/D33531#767059, @alexfh wrote: > > Would it make sense to silence this diagnostic in the presence of also > > checking for cert-dcl21-cpp for such operators? > > Currently there's no mechanism in clang-tidy to express dependencies or > compatibility issues between checks. Moreover, we already have checks that > are not meant to be run together, for example, > readability-braces-around-statements and its google- incarnation (and other > alias checks with different settings). That said, we could whitelist postfix > increment and decrement operators in this check. Camillo, WDYT? I can imagine a generic whitelist mechanism might be useful for this check. It could even be empty by default, but the documentation could call out cert-dcl21-cpp specifically and show an example of how you can run both checks. > On a side note, the check's performance implications might be more important > than the readability aspect of dropping the `const`, so the check might be a > better fit for the `performance` category. I agree. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:27 + // skip those too. + Finder->addMatcher(functionDecl(returns(qualType( +isConstQualified(), alexfh wrote: > How about just matching definitions to avoid duplicate warnings? I'll echo this. I'd be worried about this triggering on 3rd party library headers that the user cannot control, otherwise. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:97 + + auto ReturnType = Func->getReturnType(); + if (!ReturnType.isLocalConstQualified()) No `auto` here, or elsewhere, when the type isn't explicitly spelled out in the initialization. Also, you can drop the local `const` qualifiers on value types. Repository: rL LLVM https://reviews.llvm.org/D33531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33531: Clang-tidy readability: avoid const value return
alexfh added a comment. > Would it make sense to silence this diagnostic in the presence of also > checking for cert-dcl21-cpp for such operators? Currently there's no mechanism in clang-tidy to express dependencies or compatibility issues between checks. Moreover, we already have checks that are not meant to be run together, for example, readability-braces-around-statements and its google- incarnation (and other alias checks with different settings). That said, we could whitelist postfix increment and decrement operators in this check. Camillo, WDYT? On a side note, the check's performance implications might be more important than the readability aspect of dropping the `const`, so the check might be a better fit for the `performance` category. Repository: rL LLVM https://reviews.llvm.org/D33531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33531: Clang-tidy readability: avoid const value return
aaron.ballman added a comment. I'm not opposed to this check, but I'm not keen on having a check that directly contradicts the output from another check. The `cert-dcl21-cpp` check will diagnose user's code when a postfix operator ++ or -- is *not* const-qualified. Would it make sense to silence this diagnostic in the presence of also checking for `cert-dcl21-cpp` for such operators? Repository: rL LLVM https://reviews.llvm.org/D33531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33531: Clang-tidy readability: avoid const value return
alexfh added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:27 + // skip those too. + Finder->addMatcher(functionDecl(returns(qualType( +isConstQualified(), How about just matching definitions to avoid duplicate warnings? Comment at: test/clang-tidy/readability-const-value-return.cpp:1 +// RUN: %check_clang_tidy %s readability-const-value-return %t + Please add tests for: 1. a function with multiple declarations and a definition 2. function declarations/definitions in macros 3. a class method 4. implicit stuff (I'm not sure if a lambda can be made to return a const type without an explicit return type specification) Comment at: test/clang-tidy/readability-const-value-return.cpp:50 +template +const T f_returns_template_param(); + It would be nice to ensure the check doesn't trigger on template instantiation of this function as well. Repository: rL LLVM https://reviews.llvm.org/D33531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33531: Clang-tidy readability: avoid const value return
alexfh added a comment. > The check ignores returns of const pointers (meaning * const, not const *); > while pointless, these are not particularly harmful. It could be made laxer > by ignoring all trivially copyable types (on the grounds that they won't have > interesting move constructors/assigners anyway), or stricter by ignoring all > const returns (on the grounds that, in the best case, it's just pointless > verbosity). Or it could be made an option. I'd prefer an option. It could be `StrictMode`, which is also supported as a global option, for example, by clang-tidy/misc/ArgumentCommentCheck.cpp. Repository: rL LLVM https://reviews.llvm.org/D33531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
