Commit c375dc23 authored by Alexander Kornienko's avatar Alexander Kornienko
Browse files

Revert "Fix llvm-namespace-comment for macro expansions" 

This reverts commit 4736d63f.
This commit introduces a ton of false positives and incorrect fixes. See https://reviews.llvm.org/D69855#1767089 for details.
parent 8a5b7c35

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

+13 −120
Original line number Diff line number Diff line
@@ -19,44 +19,6 @@ 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),
@@ -78,36 +40,23 @@ 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);

}



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

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

                                       bool InsertLineBreak) {

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

  if (!ND->isAnonymousNamespace()) {

    bool IsNamespaceMacroExpansion;

    StringRef MacroDefinition;

    std::tie(IsNamespaceMacroExpansion, MacroDefinition) =

        isNamespaceMacroExpansion(ND->getName());



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

                                                     : ND->getName());

  }

  if (!ND->isAnonymousNamespace())

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

  if (InsertLineBreak)

    Fix.append("\n");

  return Fix;

}



std::string

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

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

                                       bool InsertLineBreak) {

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

  Fix.append(NameSpaceName);
@@ -116,32 +65,6 @@ NamespaceCommentCheck::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;
@@ -220,48 +143,28 @@ 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()) &&

                  !IsNamespaceMacroExpansion)) {

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

                  Anonymous.empty())) {

        // 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()));



      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 '") +

      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.
@@ -274,16 +177,6 @@ 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

+0 −15
Original line number Diff line number Diff line
@@ -26,29 +26,14 @@ 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' not terminated with

// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'MACRO' starts here

// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'macro_expansion' not terminated with

// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'macro_expansion' starts here

}

// CHECK-FIXES: }  // namespace MACRO

// CHECK-FIXES: }  // namespace macro_expansion



namespace short1 {

namespace short2 {

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

deleted100644 → 0
+0 −41
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