rsmith added inline comments.

================
Comment at: include/clang/AST/Stmt.h:206
+    bool PartOfExplicitCast : 1;
+    unsigned BasePathSize : 32 - 6 - 1 - NumExprBits;
   };
----------------
lebedev.ri wrote:
> rjmccall wrote:
> > lebedev.ri wrote:
> > > rjmccall wrote:
> > > > This needs to be serialized.
> > > Uhm, could you please explain what do you mean by 'serialized'?
> > It needs to be preserved when writing an ICE into a PCH / module file.  See 
> > the ASTWriter / ASTReader.
> Aha. I did add handling there but it raises questions:
>   #  This will silently break with different AST serialization versions.
>      I'm not sure how to handle it, since `VERSION_MINOR` isn't even read 
> back.
>   #  //Does// this need a test? How to write one?
>      Like `./test/PCH/include-timestamp.cpp`, using `llvm-bcanalyzer`?
Don't worry about breaking the serialization format. We do not maintain AST 
file format compatibility in general (neither between major releases nor 
between any two arbitrary SVN revisions). [We should probably maintain file 
format compatibility between patch releases, but I don't think that works right 
now because we check the full version number including the patch level when 
reading a file.]

Please do add a test: what you need to do is create a PCH containing an 
implicit cast expression and then import that AST file and do anything to check 
that the value is preserved (such as inspecting the output of `-ast-dump`). 
Maybe you could add this to the existing `test/PCH/cxx_exprs.cpp` test, which 
already does most of what you want, but doesn't have any `FileCheck` tests on 
the `-ast-dump` output yet.


================
Comment at: lib/AST/ASTDumper.cpp:2122
+  if (Node->getIsPartOfExplicitCast())
+    OS << " PartOfExplicitCast";
 }
----------------
Our predominant convention is to use lower_snake_case for such things in 
`-ast-dump` (though we're not exactly consistent on this...)


================
Comment at: lib/Sema/SemaCast.cpp:94-101
+    void updatePartOfExplicitCastFlags(CastExpr *CE) {
+      // Walk down from the CE to the OrigSrcExpr, and mark all immediate
+      // ImplicitCastExpr's as being part of ExplicitCastExpr. The original CE
+      // (which is a ExplicitCastExpr), and the OrigSrcExpr are not touched.
+      while (OrigSrcExpr.get() != CE->getSubExpr() &&
+             (CE = dyn_cast<ImplicitCastExpr>(CE->getSubExpr())))
+        CE->setIsPartOfExplicitCast(true);
----------------
You don't need to track the `OrigSrcExpr` here. You can just recurse down 
through all the `ImplicitCastExpr`s (they're always all notionally part of the 
explicit cast).


Repository:
  rC Clang

https://reviews.llvm.org/D49508



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to