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

[Firat Kasmis via Phabricator via cfe-commits]

firolino added inline comments.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:153
+  const SourceRange FVLoc(DeclStmt->getLocStart(), Location);
+  std::string UserWrittenType =
+      Lexer::getSourceText(CharSourceRange::getCharRange(FVLoc), SM,
----------------
alexfh wrote:
> 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?
Sounds interesting! I am gonna give this a try.


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


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.h:25
+class OneNamePerDeclarationCheck : public ClangTidyCheck {
+private:
+  std::string getUserWrittenType(const DeclStmt *DeclStmt, SourceManager &SM);
----------------
alexfh wrote:
> nit: move the private section to the end.
[[ http://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords | LLVM Coding Standards ]] is putting the private section at the beginning as well. Even in the code base, it seems to be the general rule. While you are reading code from top to bottom, you need to get the necessary context (variables and their types) first, before proceeding to the functional code. Otherwise, you would have to scroll all the time down to the private variables - when passing its usage in a function - to get its type. This would impair the reading fluency and thus the straight-forwarding understanding of the code.


================
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);
+
----------------
alexfh wrote:
> 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?
More efficient? In worst-case (token not found) yes, in my use-case probably not that significant. You are right, changing the interface to `Token findTokenBackward(...)` seems to be more generic than returning the location directly. 


https://reviews.llvm.org/D27621