[PATCH] D55534: [AST] Store "UsesADL" information in CallExpr.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 13 inline comments as done.
EricWF added inline comments.


================
Comment at: include/clang/Sema/Sema.h:2758
                             bool AllowExplicit = false,
+                            bool IsADLCandidate = false,
                             ConversionSequenceList EarlyConversions = None);
----------------
rsmith wrote:
> Long lists of bool arguments make me nervous, especially with default arguments. Can you introduce an enum for the new param at least?
Ack. I was thinking the same thing. 


================
Comment at: lib/AST/ASTImporter.cpp:7387
   return new (Importer.getToContext()) CallExpr(
       Importer.getToContext(), ToCallee, ToArgs, ToType, E->getValueKind(),
       ToRParenLoc);
----------------
rsmith wrote:
> Do we need to pass through the usesADL flag here too?
Seems like it. I'll add tests as well.


================
Comment at: lib/AST/Expr.cpp:1267
     : Expr(SC, Empty), NumArgs(NumArgs) {
+  CallExprBits.UsesADL = false;
   CallExprBits.NumPreArgs = NumPreArgs;
----------------
riccibruno wrote:
> riccibruno wrote:
> > I believe that msan can cope with bit level operations just fine.
> > What I am afraid is that initializing it here will hide the
> > fact that it is not initialized during deserialization if the
> > `E->setUsesADL(Record.readInt());` is removed by mistake.
> > 
> > At least not initializing it here will leave a chance for msan
> > to catch this.
> I don't think they do.
Regardless. I'll remove the initialization.

I think potentially getting a random value will make it easier to spot bugs.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55534/new/

https://reviews.llvm.org/D55534