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

Firat Kasmis via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Dec 28 07:09:39 PST 2016


firolino marked 27 inline comments as done.
firolino added inline comments.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:34
+  // a tag declaration (e.g. struct, class etc.):
+  // class A { } Object1, Object2;  <-- won't be matched
+  Finder->addMatcher(
----------------
aaron.ballman wrote:
> firolino wrote:
> > aaron.ballman wrote:
> > > Why do we not want to match this?
> > If we decide, whether we transform 
> > ```
> > class A { 
> > } Object1, Object2;
> > ``` 
> > to
> > ```
> > class A { 
> > } Object1, 
> > Object2;
> > ``` 
> > or
> > ```
> > class A { 
> > } 
> > Object1, 
> > Object2;
> > ``` 
> > I might consider adding support for it. Moreover, this kind of definition is usually seen globally and I don't know how to handle globals yet. See http://lists.llvm.org/pipermail/cfe-dev/2015-November/046262.html
> I think this should be handled. It can be handled in either of the forms you show, or by saying:
> ```
> A Object1;
> A Object2;
> ```
> If all of these turn out to be a problem, we can still diagnose without providing a fixit.
> 
> As for globals in general, they can be handled in a separate patch once we figure out the declaration grouping.
OK. I will try to split the object definition from the class definition, as you have suggested. Thus, I can kick out the tagDecl-matcher as well. If there is no easy way to do this, it will be reported anyway but without a fixit.

Note for me: Update documentation!


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:101
+  QualType Type;
+  if (const auto *FirstVar = dyn_cast<const DeclaratorDecl>(*FirstVarIt)) {
+    Location = FirstVar->getLocation();
----------------
aaron.ballman wrote:
> firolino wrote:
> > aaron.ballman wrote:
> > > You can drop the `const` from the `dyn_cast`, here and elsewhere.
> > Alright, but can you explain why?
> The `const` on the declaration is sufficient to ensure const correctness. It's morally equivalent to:
> ```
> void f(int i) {
>   const int ci = i; // No need to const_cast this either.
> }
> ```
Ah, got it. Thought first, you meant to remove any const in that dyn_cast line.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:137
+    while (Type->isAnyPointerType() || Type->isArrayType())
+      Type = Type->getPointeeOrArrayElementType()->getCanonicalTypeInternal();
+  }
----------------
aaron.ballman wrote:
> firolino wrote:
> > aaron.ballman wrote:
> > > Why are you calling the Internal version of this function?
> > Because I need its QualType.
> You should not call functions that have Internal in the name (generally speaking). Those are generally considered to be warts we want to remove once we get rid of the last external caller.
> 
> All of your uses of `Type` are as a `clang::Type *`, so are you sure you need the `QualType` at all? (Btw, you should consider a better name than `Type`.)
operator-> is overloaded in QualType... I was so confused all the time. Changed completely to Type.


https://reviews.llvm.org/D27621





More information about the cfe-commits mailing list