davrec added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:59
+ RTy->getCanonicalTypeUnqualified();
+
+ // FIXME: We should probably check the other kinds of nested name specifiers.
----------------
steakhal wrote:
> davrec wrote:
> > It occurs to me this needs to be recursive, to check a sequence of
> > qualifiers, i.e.
> > ```
> > const Type *LTy = Left->getAsType();
> > const Type *RTy = Right->getAsType();
> > if (!LTy || !RTy)
> > return true;
> > if (LTy->getCanonicalTypeUnqualified() !=
> > RTy->getCanonicalTypeUnqualified())
> > return false;
> > return areEquivalentNameSpecifier(Left->getPrefix(), Right->getPrefix());
> > ```
> >
> > The reason is, if there is a prefix qualifier to this qualifier, we run
> > into the original problem, e.g.:
> > ```
> > struct Base {
> > struct Inner {
> > static const bool value = true;
> > };
> > };
> > struct Outer1 : Base {};
> > struct Outer2 : Base {};
> >
> > // We do not want the following to warn, but without checking the prefix of
> > Inner,
> > // I believe these will be interpreted as equivalent DeclRefs and will warn:
> > auto sink = Outer1::Inner::value || Outer2::Inner::value;
> > ```
> >
> You are right, but that would require me to implement all the possible kinds
> of NNS, in addition to the types.
> And it's not entirely clear to me (yet) what the desired behavior should be
> for implementing this.
I agree it's not entirely clear what should be the desired behavior in that
rare case; I suppose it might be appropriate to introduce a `bool
RecurseToPrefix` param that determines whether you do the while loop or finish
after one iteration, mostly so that others using this function can ponder the
decision for themselves.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp:372
+}
+
/// Determines whether two statement trees are identical regarding
----------------
I would strongly support turning these functions into methods of DeclRefExpr
and NestedNameSpecifier, not just to avoid the code duplication here but
because they sure seem to be of general utility to AST users.
You just need to clearly motivate these methods in the documentation, which
your description for this patch largely does, e.g. something like.:
```
class DeclRefExpr : ... {
...
/// Returns whether this \c DeclRefExpr
/// a. refers to the same canonical declaration as \c other and
/// b. if it has a qualifier, that qualifier refers to the same canonical
/// declarations as those of \c other .
///
/// Consider these two DeclRefExprs:
/// \code
/// std::is_same<char, int>::value
/// std::is_same<char, long>::value;
/// \endcode
///
/// We can see that the value static members are different, but in fact
/// the \c getDecl() of these two returns the same VarDecl. (...etc)
bool isSemanticallyEquivalentTo(const DeclRefExpr *Other) { ... }
};
...
class NestedNameSpecifier {
...
/// Returns whether this refers to the same sequence of identifiers/canonical
declarations
/// as \c Other. (Then I would repeat the std::is_same example here, since
that
/// really makes the issue clear. And also describe what this returns when
other is nullptr or
/// when getPrefix() is nullptr while other->getPrefix() is non-null -- maybe
add a parameter if
/// its not clear in general what the behavior should be.)
bool isSemanticallyEquivalentTo(const NestedNameSpecifier *Other, bool
RecurseToPrefix = true) { ... }
};
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114622/new/
https://reviews.llvm.org/D114622
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
