[PATCH] D27621: [clang-tidy] check to find declarations declaring more than one name

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

A few more comments.



================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:153
+  const SourceRange FVLoc(DeclStmt->getLocStart(), Location);
+  std::string UserWrittenType =
+      Lexer::getSourceText(CharSourceRange::getCharRange(FVLoc), SM,
----------------
I wonder whether re-lexing the whole range and working on tokens instead would be a better and more robust approach? You wouldn't have to deal with whitespace and comments then. What do you think?


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:163
+  // const int *&   -> const int
+  // long **        -> long int
+  size_t Pos = std::string::npos;
----------------
Why do we want `long int` and not `long`?


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.h:25
+class OneNamePerDeclarationCheck : public ClangTidyCheck {
+private:
+  std::string getUserWrittenType(const DeclStmt *DeclStmt, SourceManager &SM);
----------------
nit: move the private section to the end.


================
Comment at: clang-tidy/utils/LexerUtils.cpp:41-56
+  const auto &SM = Context.getSourceManager();
+  const auto &LO = Context.getLangOpts();
+  auto StartOfFile = SM.getLocForStartOfFile(SM.getFileID(Location));
+
+  Token CurrentToken;
+  CurrentToken.setKind(tok::unknown);
+
----------------
Is it significantly more efficient than

  Token Tok;
  do {
    Tok = getPreviousNonCommentToken(Context, Location);
  } while (!Tok.isOneOf(tok::unknown, TokenToFind));
  return Tok.getLocation();

?

If it's not, then this function might be not so much useful to be here. At least, its interface is rather specific to the use case you have. Why, for example, it returns a location instead of the token itself?


https://reviews.llvm.org/D27621