carlosgalvezp added a comment. Great feedback, thanks! I had some ideas on how to go around the issues, looking forward to your thoughts.
================ Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp:103-104 +struct WithAlias { + ConstType c; + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified + RefType lr; ---------------- njames93 wrote: > I feel like this could potentially confuse users. Maybe we should walk the > alias to show where the type was declared to be const/reference. > Or, not a fan of this choice, don't diagnose these unless the user opts into > this behaviour. Sounds reasonable! I see that Clang compiler does not print an "alias stack" in detail, for example here: https://godbolt.org/z/8sqE4fM1v For the sake of consistency and simplicity, would it make sense to print something similar here? Example: "warning: member 'c' of type 'ConstType' (aka 'const int') is const qualified" ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp:136-157 +template <typename T> +struct TemplatedOk { + T t; +}; + +template <typename T> +struct TemplatedConst { ---------------- njames93 wrote: > This whole block displeases me. Warning on the instantiation isn't a great > idea and could confuse users. > Would again need some expansion notes to explain why the warning is being > triggered, especially when if for 99% of the uses one of these structs has a > T which isn't const or a reference. > Failing that just disabling the check in template instantiations would also > fix the issue. I agree with the sentiment 100%. Unfortunately it's currently the only way to test this kind of code in clang-tidy AFAIK, see for example other existing checks: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/google/readability-casting.cpp#L332 I fully agree on needing expansion notes, just like the Clang compiler does. A very similar question about it was recently posted: https://discourse.llvm.org/t/clang-tidy-diagnostics-for-template-code/62909 In that sense I believe it would make sense to try and focus the efforts into implementing a good centralized solution instead of having it duplicated on different checks. Until then, I believe it should be fairly easy to clarify the actual type as proposed in the above comment for aliases. What do you think? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits