Commit 4736d63f authored by Marcin Twardak's avatar Marcin Twardak Committed by Aaron Ballman
Browse files

Fix llvm-namespace-comment for macro expansions 

If a namespace is a macro name, it should be allowed to close the
namespace with the same name.
parent 7a0c5484

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

+120 −13
Original line number Diff line number Diff line
@@ -19,6 +19,44 @@ namespace clang { 
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 * 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,23 +78,36 @@ void NamespaceCommentCheck::registerMatchers(MatchFinder *Finder) { 
    Finder->addMatcher(namespaceDecl().bind("namespace"), this);

}



void NamespaceCommentCheck::registerPPCallbacks(

    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {

  PP->addPPCallbacks(std::make_unique<NamespaceCommentPPCallbacks>(PP, this));

}



static bool locationsInSameFile(const SourceManager &Sources,

                                SourceLocation Loc1, SourceLocation Loc2) {

  return Loc1.isFileID() && Loc2.isFileID() &&

         Sources.getFileID(Loc1) == Sources.getFileID(Loc2);

}



static std::string getNamespaceComment(const NamespaceDecl *ND,

std::string NamespaceCommentCheck::getNamespaceComment(const NamespaceDecl *ND,

                                                       bool InsertLineBreak) {

  std::string Fix = "// namespace";

  if (!ND->isAnonymousNamespace())

    Fix.append(" ").append(ND->getNameAsString());

  if (!ND->isAnonymousNamespace()) {

    bool IsNamespaceMacroExpansion;

    StringRef MacroDefinition;

    std::tie(IsNamespaceMacroExpansion, MacroDefinition) =

        isNamespaceMacroExpansion(ND->getName());



    Fix.append(" ").append(IsNamespaceMacroExpansion ? MacroDefinition

                                                     : ND->getName());

  }

  if (InsertLineBreak)

    Fix.append("\n");

  return Fix;

}



static std::string getNamespaceComment(const std::string &NameSpaceName,

std::string

NamespaceCommentCheck::getNamespaceComment(const std::string &NameSpaceName,

                                           bool InsertLineBreak) {

  std::string Fix = "// namespace ";

  Fix.append(NameSpaceName);
@@ -65,6 +116,32 @@ static std::string getNamespaceComment(const std::string &NameSpaceName, 
  return Fix;

}



void NamespaceCommentCheck::addMacro(const std::string &Name,

                                     const std::string &Value) noexcept {

  Macros.emplace_back(Name, Value);

}



bool NamespaceCommentCheck::isNamespaceMacroDefinition(

    const StringRef NameSpaceName) {

  return llvm::any_of(Macros, [&NameSpaceName](const auto &Macro) {

    return NameSpaceName == Macro.first;

  });

}



std::tuple<bool, StringRef> NamespaceCommentCheck::isNamespaceMacroExpansion(

    const StringRef NameSpaceName) {

  const auto &MacroIt =

      llvm::find_if(Macros, [&NameSpaceName](const auto &Macro) {

        return NameSpaceName == Macro.second;

      });



  const bool IsNamespaceMacroExpansion = Macros.end() != MacroIt;



  return std::make_tuple(IsNamespaceMacroExpansion,

                         IsNamespaceMacroExpansion ? StringRef(MacroIt->first)

                                                   : NameSpaceName);

}



void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {

  const auto *ND = Result.Nodes.getNodeAs<NamespaceDecl>("namespace");

  const SourceManager &Sources = *Result.SourceManager;
@@ -143,28 +220,48 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) { 
      StringRef NamespaceNameInComment = Groups.size() > 5 ? Groups[5] : "";

      StringRef Anonymous = Groups.size() > 3 ? Groups[3] : "";



      // Don't allow to use macro expansion in closing comment.

      // FIXME: Use Structured Bindings once C++17 features will be enabled.

      bool IsNamespaceMacroExpansion;

      StringRef MacroDefinition;

      std::tie(IsNamespaceMacroExpansion, MacroDefinition) =

          isNamespaceMacroExpansion(NamespaceNameInComment);



      if (IsNested && NestedNamespaceName == NamespaceNameInComment) {

        // C++17 nested namespace.

        return;

      } else if ((ND->isAnonymousNamespace() &&

                  NamespaceNameInComment.empty()) ||

                 (ND->getNameAsString() == NamespaceNameInComment &&

                  Anonymous.empty())) {

                 (((ND->getNameAsString() == NamespaceNameInComment) &&

                   Anonymous.empty()) &&

                  !IsNamespaceMacroExpansion)) {

        // Check if the namespace in the comment is the same.

        // FIXME: Maybe we need a strict mode, where we always fix namespace

        // comments with different format.

        return;

      }



      // Allow using macro definitions in closing comment.

      if (isNamespaceMacroDefinition(NamespaceNameInComment))

        return;



      // Otherwise we need to fix the comment.

      NeedLineBreak = Comment.startswith("/*");

      OldCommentRange =

          SourceRange(AfterRBrace, Loc.getLocWithOffset(Tok.getLength()));

      Message =

          (llvm::Twine(

               "%0 ends with a comment that refers to a wrong namespace '") +



      if (IsNamespaceMacroExpansion) {

        Message = (llvm::Twine("%0 ends with a comment that refers to an "

                               "expansion of macro"))

                      .str();

        NestedNamespaceName = MacroDefinition;

      } else {

        Message = (llvm::Twine("%0 ends with a comment that refers to a "

                               "wrong namespace '") +

                   NamespaceNameInComment + "'")

                      .str();

      }



    } else if (Comment.startswith("//")) {

      // Assume that this is an unrecognized form of a namespace closing line

      // comment. Replace it.
@@ -177,6 +274,16 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) { 
    // multi-line or there may be other tokens behind it.

  }



  // Print Macro definition instead of expansion.

  // FIXME: Use Structured Bindings once C++17 features will be enabled.

  bool IsNamespaceMacroExpansion;

  StringRef MacroDefinition;

  std::tie(IsNamespaceMacroExpansion, MacroDefinition) =

      isNamespaceMacroExpansion(NestedNamespaceName);



  if (IsNamespaceMacroExpansion)

    NestedNamespaceName = MacroDefinition;



  std::string NamespaceName =

      ND->isAnonymousNamespace()

          ? "anonymous namespace"

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h

+15 −0
Original line number Diff line number Diff line
@@ -26,14 +26,29 @@ public: 
  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<bool, StringRef>

  isNamespaceMacroExpansion(const StringRef NameSpaceName);



  llvm::Regex NamespaceCommentPattern;

  const unsigned ShortNamespaceLines;

  const unsigned SpacesBeforeComments;

  llvm::SmallVector<SourceLocation, 4> Ends;



  // Store macros to verify that warning is not thrown when namespace name is a

  // preprocessed define.

  std::vector<std::pair<std::string, std::string>> Macros;

};



} // namespace readability

clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp

+3 −3
Original line number Diff line number Diff line
@@ -25,10 +25,10 @@ void f(); // So that the namespace isn't empty. 
// 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 {

clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp

0 → 100644
+41 −0
Original line number Diff line number Diff line 
// 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