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

[Dmitri Gribenko via Phabricator via cfe-commits]

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 and skip checking macro expansions completely. 
> If you agree I will provide a patch for that.


alexfh already provided a fix, please review and ensure that it fixes your 
case: https://reviews.llvm.org/D70974


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69855/new/

https://reviews.llvm.org/D69855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Marcin Twardak via Phabricator via cfe-commits]

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 {
  
  } // namespace macro

which is obviously wrong.

During review we decided that it would be a good idea (apparently not so good) 
to detect macro expansions and force users to use only macro definitions. This 
could be useful to avoid mixing macro definitions with macro expansions.

@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 and skip checking macro expansions completely. If 
you agree I will provide a patch for that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69855/new/

https://reviews.llvm.org/D69855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitri Gribenko via Phabricator via cfe-commits]

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 ACTION
  https://reviews.llvm.org/D69855/new/

https://reviews.llvm.org/D69855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

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 makes the check think that every namespace named `internal` has something to 
do with SOME_RANDOM_MACRO.

  $ clang_tidy -checks=-*,llvm-* /tmp/q.cc --
  1 warning generated.
  /tmp/q.cc:7:2: warning: namespace 'SOME_RANDOM_MACRO' ends with a comment 
that refers to an expansion of macro [llvm-namespace-comment]
  } // namespace internal
   ^~
// namespace SOME_RANDOM_MACRO
  /tmp/q.cc:5:11: note: namespace 'SOME_RANDOM_MACRO' starts here
  namespace internal {
^

This is definitely wrong and it introduces tons of false positives in our code. 
It seems to me that the check shouldn't look any further than what is actually 
spelled in the namespace declaration. It shouldn't try to match macros to their 
expansions or vice versa. I'll see whether I can implement this quickly, 
otherwise I'll just revert this patch to unbreak the checker.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69855/new/

https://reviews.llvm.org/D69855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

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 4736d63f752f8d13f4c6a9afd558565c32119718 
, thank 
you for the patch!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69855/new/

https://reviews.llvm.org/D69855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Marcin Twardak via Phabricator via cfe-commits]

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://reviews.llvm.org/D69855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

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 SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) 
{
+  PP->addPPCallbacks(std::make_unique(PP, this));
+}

twardakm wrote:
> aaron.ballman wrote:
> > twardakm wrote:
> > > aaron.ballman wrote:
> > > > Rather than making an entire new object for PP callbacks, why not make 
> > > > `NamespaceCommentCheck` inherit from `PPCallbacks`? It seems like it 
> > > > would simplify the interface somewhat.
> > > I think the check hast to inherit from ClangTidyCheck?
> > > 
> > > I duplicated the solution from other checks (e.g. 
> > > IdentifierNamingCheck.cpp). Could you please point to some existing check 
> > > which implements your idea?
> > I don't know if we have other checks doing this -- I was thinking of using 
> > multiple inheritance in this case, because the callbacks are effectively a 
> > mixin.
> > 
> > That said, I don't insist on this change.
> The problem which I see is that addPPCallbacks takes ownership of the object 
> passed to it. I couldn't find any other way of invoking PP callbacks, so I'll 
> leave it as is.
Ah, that makes sense. Thank you for investigating it. :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69855/new/

https://reviews.llvm.org/D69855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Marcin Twardak via Phabricator via cfe-commits]

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(PP, this));
+}

aaron.ballman wrote:
> twardakm wrote:
> > aaron.ballman wrote:
> > > Rather than making an entire new object for PP callbacks, why not make 
> > > `NamespaceCommentCheck` inherit from `PPCallbacks`? It seems like it 
> > > would simplify the interface somewhat.
> > I think the check hast to inherit from ClangTidyCheck?
> > 
> > I duplicated the solution from other checks (e.g. 
> > IdentifierNamingCheck.cpp). Could you please point to some existing check 
> > which implements your idea?
> I don't know if we have other checks doing this -- I was thinking of using 
> multiple inheritance in this case, because the callbacks are effectively a 
> mixin.
> 
> That said, I don't insist on this change.
The problem which I see is that addPPCallbacks takes ownership of the object 
passed to it. I couldn't find any other way of invoking PP callbacks, so I'll 
leave it as is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69855/new/

https://reviews.llvm.org/D69855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Marcin Twardak via Phabricator via cfe-commits]

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/NamespaceCommentCheck.cpp
  clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
  
clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
  clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp
@@ -0,0 +1,41 @@
+// RUN: %check_clang_tidy %s llvm-namespace-comment %t
+
+namespace n1 {
+namespace n2 {
+  void f();
+
+
+  // CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'n2' not terminated with a closing comment [llvm-namespace-comment]
+  // CHECK-MESSAGES: :[[@LINE+1]]:2: warning: namespace 'n1' not terminated with a closing comment [llvm-namespace-comment]
+}}
+// CHECK-FIXES: } // namespace n2
+// CHECK-FIXES: } // namespace n1
+
+#define MACRO macro_expansion
+namespace MACRO {
+  void f();
+  // CHECK-MESSAGES: :[[@LINE+1]]:1: warning: namespace 'MACRO' not terminated with a closing comment [llvm-namespace-comment]
+}
+// CHECK-FIXES: } // namespace MACRO
+
+namespace MACRO {
+  void g();
+} // namespace MACRO
+
+namespace MACRO {
+  void h();
+  // CHECK-MESSAGES: :[[@LINE+1]]:2: warning: namespace 'MACRO' ends with a comment that refers to an expansion of macro [llvm-namespace-comment]
+} // namespace macro_expansion
+// CHECK-FIXES: } // namespace MACRO
+
+namespace n1 {
+namespace MACRO {
+namespace n2 {
+  void f();
+  // CHECK-MESSAGES: :[[@LINE+3]]:1: warning: namespace 'n2' not terminated with a closing comment [llvm-namespace-comment]
+  // CHECK-MESSAGES: :[[@LINE+2]]:2: warning: namespace 'MACRO' not terminated with a closing comment [llvm-namespace-comment]
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: namespace 'n1' not terminated with a closing comment [llvm-namespace-comment]
+}}}
+// CHECK-FIXES: } // namespace n2
+// CHECK-FIXES: } // namespace MACRO
+// CHECK-FIXES: } // namespace n1
Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
@@ -25,10 +25,10 @@
 // 5
 // 6
 // 7
-// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'macro_expansion' not terminated with
-// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'macro_expansion' starts here
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'MACRO' not terminated with
+// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'MACRO' starts here
 }
-// CHECK-FIXES: }  // namespace macro_expansion
+// CHECK-FIXES: }  // namespace MACRO
 
 namespace short1 {
 namespace short2 {
Index: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
===
--- clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
+++ clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
@@ -26,14 +26,29 @@
   NamespaceCommentCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override;
+
+  void addMacro(const std::string &Name, const std::string &Value) noexcept;
 
 private:
   void storeOptions(ClangTidyOptions::OptionMap &Options) override;
+  std::string getNamespaceComment(const NamespaceDecl *ND,
+  bool InsertLineBreak);
+  std::string getNamespaceComment(const std::string &NameSpaceName,
+  bool InsertLineBreak);
+  bool isNamespaceMacroDefinition(const StringRef NameSpaceName);
+  std::tuple
+  isNamespaceMacroExpansion(const StringRef NameSpaceName);
 
   llvm::Regex NamespaceCommentPattern;
   const unsigned ShortNamespaceLines;
   const unsigned SpacesBeforeComments;
   llvm::SmallVector Ends;
+
+  // Store macros to verify that warning is not thrown when namespace name is a
+  // preprocessed define.
+  std::vector> Macros;
 };
 
 } // namespace readability
Index: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
@@ -19,6 +19,44 @@
 namespace tidy {
 

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

[Aaron Ballman via Phabricator via cfe-commits]

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));
+}

twardakm wrote:
> aaron.ballman wrote:
> > Rather than making an entire new object for PP callbacks, why not make 
> > `NamespaceCommentCheck` inherit from `PPCallbacks`? It seems like it would 
> > simplify the interface somewhat.
> I think the check hast to inherit from ClangTidyCheck?
> 
> I duplicated the solution from other checks (e.g. IdentifierNamingCheck.cpp). 
> Could you please point to some existing check which implements your idea?
I don't know if we have other checks doing this -- I was thinking of using 
multiple inheritance in this case, because the callbacks are effectively a 
mixin.

That said, I don't insist on this change.



Comment at: 
clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:101-104
+if (!IsNamespaceMacroExpansion)
+  Fix.append(" ").append(ND->getNameAsString());
+else
+  Fix.append(" ").append(MacroDefinition);

Would this work instead `Fix.append(" ").append(IsNamespaceMacroExpansion ? 
MacroDefinition : ND->getName());`?

(You may have to check the behavioral difference of `getName()` vs 
`getNameAsString()` to be sure.)



Comment at: 
clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:135
+const StringRef NameSpaceName) {
+  const auto &MacroIt = std::find_if(std::begin(Macros), std::end(Macros),
+ [&NameSpaceName](const auto &Macro) {

`llvm::find_if(Macros, ...);`



Comment at: 
clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:247
+  // Allow using macro definitions in closing comment.
+  if (isNamespaceMacroDefinition(NamespaceNameInComment)) {
+return;

Can elide braces.



Comment at: 
clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:287
+
+  if (IsNamespaceMacroExpansion) {
+NestedNamespaceName = MacroDefinition;

Elide braces


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69855/new/

https://reviews.llvm.org/D69855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Marcin Twardak via Phabricator via cfe-commits]

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
+
+Check->addMacro(MacroNameTok.getIdentifierInfo()->getName().str(),
+Value.str());
+  }

aaron.ballman wrote:
> You only need to add the macro name in this case, not its value, which should 
> simplify this code considerably.
See comment above, now both value and definition is used



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));
+}

aaron.ballman wrote:
> Rather than making an entire new object for PP callbacks, why not make 
> `NamespaceCommentCheck` inherit from `PPCallbacks`? It seems like it would 
> simplify the interface somewhat.
I think the check hast to inherit from ClangTidyCheck?

I duplicated the solution from other checks (e.g. IdentifierNamingCheck.cpp). 
Could you please point to some existing check which implements your idea?



Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h:44
+  // preprocessed define
+  std::vector> Macros;
 };

aaron.ballman wrote:
> I think a better approach is to use a set of some sort because the value of 
> the macro is never used in the check. Probably a `SmallPtrSet` over 
> `StringRef`.
After applying other comments, both value and definition is used, therefore I 
stayed with pair of strings. Let me know if you think this comment is still 
valid


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69855/new/

https://reviews.llvm.org/D69855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Marcin Twardak via Phabricator via cfe-commits]

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.llvm.org/D69855/new/

https://reviews.llvm.org/D69855

Files:
  clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
  clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
  
clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
  clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp
@@ -0,0 +1,41 @@
+// RUN: %check_clang_tidy %s llvm-namespace-comment %t
+
+namespace n1 {
+namespace n2 {
+  void f();
+
+
+  // CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'n2' not terminated with a closing comment [llvm-namespace-comment]
+  // CHECK-MESSAGES: :[[@LINE+1]]:2: warning: namespace 'n1' not terminated with a closing comment [llvm-namespace-comment]
+}}
+// CHECK-FIXES: } // namespace n2
+// CHECK-FIXES: } // namespace n1
+
+#define MACRO macro_expansion
+namespace MACRO {
+  void f();
+  // CHECK-MESSAGES: :[[@LINE+1]]:1: warning: namespace 'MACRO' not terminated with a closing comment [llvm-namespace-comment]
+}
+// CHECK-FIXES: } // namespace MACRO
+
+namespace MACRO {
+  void g();
+} // namespace MACRO
+
+namespace MACRO {
+  void h();
+  // CHECK-MESSAGES: :[[@LINE+1]]:2: warning: namespace 'MACRO' ends with a comment that refers to an expansion of macro [llvm-namespace-comment]
+} // namespace macro_expansion
+// CHECK-FIXES: } // namespace MACRO
+
+namespace n1 {
+namespace MACRO {
+namespace n2 {
+  void f();
+  // CHECK-MESSAGES: :[[@LINE+3]]:1: warning: namespace 'n2' not terminated with a closing comment [llvm-namespace-comment]
+  // CHECK-MESSAGES: :[[@LINE+2]]:2: warning: namespace 'MACRO' not terminated with a closing comment [llvm-namespace-comment]
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: namespace 'n1' not terminated with a closing comment [llvm-namespace-comment]
+}}}
+// CHECK-FIXES: } // namespace n2
+// CHECK-FIXES: } // namespace MACRO
+// CHECK-FIXES: } // namespace n1
Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
@@ -25,10 +25,10 @@
 // 5
 // 6
 // 7
-// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'macro_expansion' not terminated with
-// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'macro_expansion' starts here
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'MACRO' not terminated with
+// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'MACRO' starts here
 }
-// CHECK-FIXES: }  // namespace macro_expansion
+// CHECK-FIXES: }  // namespace MACRO
 
 namespace short1 {
 namespace short2 {
Index: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
===
--- clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
+++ clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
@@ -26,14 +26,29 @@
   NamespaceCommentCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override;
+
+  void addMacro(const std::string &Name, const std::string &Value) noexcept;
 
 private:
   void storeOptions(ClangTidyOptions::OptionMap &Options) override;
+  std::string getNamespaceComment(const NamespaceDecl *ND,
+  bool InsertLineBreak);
+  std::string getNamespaceComment(const std::string &NameSpaceName,
+  bool InsertLineBreak);
+  bool isNamespaceMacroDefinition(const StringRef NameSpaceName);
+  std::tuple
+  isNamespaceMacroExpansion(const StringRef NameSpaceName);
 
   llvm::Regex NamespaceCommentPattern;
   const unsigned ShortNamespaceLines;
   const unsigned SpacesBeforeComments;
   llvm::SmallVector Ends;
+
+  // Store macros to verify that warning is not thrown when namespace name is a
+  // preprocessed define.
+  std::vector> Macros;
 };
 
 } // namespace readability
Index: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/NamespaceComme

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

[Aaron Ballman via Phabricator via cfe-commits]

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 mismatches in other configuration modes.




Comment at: 
clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:30
+// Record all defined macros. We store the whole token to compare names
+// later
+

Missing full stop.



Comment at: 
clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:32
+
+const MacroInfo *const MI = MD->getMacroInfo();
+

You can drop the latter `const` (we don't do top-level `const` qualification 
with any consistency).



Comment at: 
clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:50-51
+
+Check->addMacro(MacroNameTok.getIdentifierInfo()->getName().str(),
+Value.str());
+  }

You only need to add the macro name in this case, not its value, which should 
simplify this code considerably.



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));
+}

Rather than making an entire new object for PP callbacks, why not make 
`NamespaceCommentCheck` inherit from `PPCallbacks`? It seems like it would 
simplify the interface somewhat.



Comment at: 
clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:208-215
+  std::find_if(std::begin(Macros), std::end(Macros),
+   [&NamespaceNameInComment](const auto &Macro) {
+ return NamespaceNameInComment == Macro.first;
+   });
+
+  if (MacroIt != Macros.end()) {
+return;

Rather than this, I'd suggest using `llvm::any_of()`.



Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h:43
+  // Store macros to verify that warning is not thrown when namespace name is a
+  // preprocessed define
+  std::vector> Macros;

Missing a full stop at the end of the sentence.



Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h:44
+  // preprocessed define
+  std::vector> Macros;
 };

I think a better approach is to use a set of some sort because the value of the 
macro is never used in the check. Probably a `SmallPtrSet` over `StringRef`.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp:19
+}
+// CHECK-FIXES: } // namespace macro_expansion
+

This seems unintuitive to me -- it should suggest the macro name instead.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp:27
+  void h();
+} // namespace macro_expansion

This also seems unintuitive to me, I would have wanted this to diagnose and 
provide a fixit for suggesting the macro name, not its expansion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69855/new/

https://reviews.llvm.org/D69855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Marcin Twardak via Phabricator via cfe-commits]

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/NamespaceCommentCheck.cpp
  clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
  clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s llvm-namespace-comment %t
+
+namespace n1 {
+namespace n2 {
+  void f();
+
+
+  // CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'n2' not terminated with a closing comment [llvm-namespace-comment]
+  // CHECK-MESSAGES: :[[@LINE+1]]:2: warning: namespace 'n1' not terminated with a closing comment [llvm-namespace-comment]
+}}
+// CHECK-FIXES: } // namespace n2
+// CHECK-FIXES: } // namespace n1
+
+#define MACRO macro_expansion
+namespace MACRO {
+  void f();
+  // CHECK-MESSAGES: :[[@LINE+1]]:1: warning: namespace 'macro_expansion' not terminated with a closing comment [llvm-namespace-comment]
+}
+// CHECK-FIXES: } // namespace macro_expansion
+
+namespace MACRO {
+  void g();
+} // namespace MACRO
+
+namespace MACRO {
+  void h();
+} // namespace macro_expansion
Index: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
===
--- clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
+++ clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
@@ -26,6 +26,10 @@
   NamespaceCommentCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override;
+
+  void addMacro(const std::string &Name, const std::string &Value) noexcept;
 
 private:
   void storeOptions(ClangTidyOptions::OptionMap &Options) override;
@@ -34,6 +38,10 @@
   const unsigned ShortNamespaceLines;
   const unsigned SpacesBeforeComments;
   llvm::SmallVector Ends;
+
+  // Store macros to verify that warning is not thrown when namespace name is a
+  // preprocessed define
+  std::vector> Macros;
 };
 
 } // namespace readability
Index: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
@@ -19,6 +19,44 @@
 namespace tidy {
 namespace readability {
 
+namespace {
+class NamespaceCommentPPCallbacks : public PPCallbacks {
+public:
+  NamespaceCommentPPCallbacks(Preprocessor *PP, NamespaceCommentCheck *Check)
+  : PP(PP), Check(Check) {}
+
+  void MacroDefined(const Token &MacroNameTok, const MacroDirective *MD) {
+// Record all defined macros. We store the whole token to compare names
+// later
+
+const MacroInfo *const MI = MD->getMacroInfo();
+
+if (MI->isFunctionLike())
+  return;
+
+std::string ValueBuffer;
+llvm::raw_string_ostream Value(ValueBuffer);
+
+SmallString<128> SpellingBuffer;
+bool First = true;
+for (const auto &T : MI->tokens()) {
+  if (!First && T.hasLeadingSpace())
+Value << ' ';
+
+  Value << PP->getSpelling(T, SpellingBuffer);
+  First = false;
+}
+
+Check->addMacro(MacroNameTok.getIdentifierInfo()->getName().str(),
+Value.str());
+  }
+
+private:
+  Preprocessor *PP;
+  NamespaceCommentCheck *Check;
+};
+} // namespace
+
 NamespaceCommentCheck::NamespaceCommentCheck(StringRef Name,
  ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
@@ -40,6 +78,11 @@
 Finder->addMatcher(namespaceDecl().bind("namespace"), this);
 }
 
+void NamespaceCommentCheck::registerPPCallbacks(
+const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(std::make_unique(PP, this));
+}
+
 static bool locationsInSameFile(const SourceManager &Sources,
 SourceLocation Loc1, SourceLocation Loc2) {
   return Loc1.isFileID() && Loc2.isFileID() &&
@@ -65,6 +108,11 @@
   return Fix;
 }
 
+void NamespaceCommentCheck::addMacro(const std::string &Name,
+ const std::string &Value) noexcept {
+  Macros.emplace_back(Name, Value);
+}
+
 void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *ND = Result.Nodes.getNodeAs("namespace");
   const SourceManager &Sources = *Re

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

[Eugene Zelenko via Phabricator via cfe-commits]

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

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69855/new/

https://reviews.llvm.org/D69855



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits