[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

gribozavr2 added a comment. In D69855#1769318 , @twardakm wrote: > @alexfh, @gribozavr2, @aaron.ballman I think the best way out here is just to > implement the basic fix for the above problem and only allow to use macro > definition in closing comment a

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

twardakm added a comment. The reason for this whole patch was a bug in clang-tidy, which caused warning in the following code: #define SOME_RANDOM_MACRO macro namespace SOME_RANDOM_MACRO { } // namespace SOME_RANDOM_MACRO and clang-tidy suggested: namespace SOME_RANDOM_MACRO {

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

gribozavr2 added a comment. @twardakm: I'm not convinced this feature is worth implementing at all, because there's a good alternative to a macro here -- a namespace alias. What is the reason to use a macro instead of a namespace alias? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

alexfh added a comment. There's a problem with this patch. Consider the following code: // In some remote header: #define SOME_RANDOM_MACRO internal // In a completely unrelated file that transitively includes the header: namespace internal { void f(); } // namespace internal It m

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

aaron.ballman closed this revision. aaron.ballman added a comment. In D69855#1754047 , @twardakm wrote: > @aaron.ballman thanks for the review :) Can you please push the change on my > behalf? I don't have commit rights. I've commit on your behalf in 47

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

twardakm marked an inline comment as done. twardakm added a comment. @aaron.ballman thanks for the review :) Can you please push the change on my behalf? I don't have commit rights. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69855/new/ https://

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

aaron.ballman accepted this revision. aaron.ballman marked an inline comment as done. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:83 +const SourceManage

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

twardakm marked 9 inline comments as done. twardakm added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:83 +const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) { + PP->addPPCallbacks(std::make_unique(

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

twardakm updated this revision to Diff 230285. twardakm added a comment. Fix style issues Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69855/new/ https://reviews.llvm.org/D69855 Files: clang-tools-extra/clang-tidy/readability/NamespaceCommentCh

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:83 +const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) { + PP->addPPCallbacks(std::make_unique(PP, this)); +} twarda

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

twardakm marked 9 inline comments as done. twardakm added a comment. Thanks for the review! @aaron.ballman take a look at new revision and let me know if something needs to be fixed. Thanks! Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:50-51

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

twardakm updated this revision to Diff 230113. twardakm added a comment. Apply Aaron's comments - Fix missing namespace to macro definition instead of extension - Print macro definition also in warning message Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

aaron.ballman added a comment. There's a design question about why it should accept the expanded macro name instead of requiring the user to write the macro. I am worried about allowing the expanded form because the presumable use of a macro is to control the name, so this invites name mismatch

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

twardakm updated this revision to Diff 228088. twardakm added a comment. Remove unnecessary line Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69855/new/ https://reviews.llvm.org/D69855 Files: clang-tools-extra/clang-tidy/readability/NamespaceCo

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:226 .str(); + } else if (Comment.startswith("//")) { Is this empty line necessary? Repository: rG LLVM Github Monorepo C