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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
