vleschuk marked an inline comment as done.
vleschuk added inline comments.

================
Comment at: include/clang/AST/ASTContext.h:83
     uint64_t Width;
-    unsigned Align;
+    llvm::DIAlignment Align;
     bool AlignIsRequired : 1;
----------------
vleschuk wrote:
> aprantl wrote:
> > I'm not sure we want to use a debug info type inside the AST. I think we 
> > only want to use them in CGDebugInfo.cpp.
> We use TypeInfo and related functions heavily in CGDebugInfo.cpp, leaving 
> this field as "unsigned" will make us to perform conversions from "unsigned" 
> to llvm::DIAlignment, I think having this changed will result in simpler code.
@dblaikie wrote:
> It'd still be a layering violation to have AST depend on debug info 
> types/concepts. If it makes sense to change it to uint32_t in AST, that'd 
> probably be reasonable.

We are discussing the possibility of changing this to "unsigned" in 
https://reviews.llvm.org/D25620, after we come to consensus I'll update both 
patches. Thanks for the advice, I agree that using DebugInfo types in AST was a 
bad idea. 



https://reviews.llvm.org/D25621



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

Reply via email to