[PATCH] D16135: Macro Debug Info support in Clang

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/CodeGen/ModuleBuilder.h:71
+  ///
+  /// This methods can be called before initializing the CGDebugInfo calss.
+  /// But the returned value should not be used until after initialization.
----------------
Typo 'calss'


================
Comment at: include/clang/CodeGen/ModuleBuilder.h:72
+  /// This methods can be called before initializing the CGDebugInfo calss.
+  /// But the returned value should not be used until after initialization.
+  /// It is caller responsibility to validate that the place holder was
----------------
What is the returned value in that case? A reference to a null pointer?

Generally, I'm not particularly happy with this approach of exposing a reference to an internal pointer as the way of propagating the `CGDebugInfo` to the macro generator as needed. Instead, how about giving the `MacroPPCallbacks` object a pointer to the `CodeGenerator` itself, and having it ask the `CodeGenModule` for the debug info object when it needs it?


================
Comment at: include/clang/CodeGen/ModuleBuilder.h:73
+  /// But the returned value should not be used until after initialization.
+  /// It is caller responsibility to validate that the place holder was
+  /// initialized before start using it.
----------------
caller -> the caller's


================
Comment at: include/clang/CodeGen/ModuleBuilder.h:74
+  /// It is caller responsibility to validate that the place holder was
+  /// initialized before start using it.
+  CodeGen::CGDebugInfo *&CodeGenerator::getModuleDebugInfoRef();
----------------
start using -> starting to use


================
Comment at: lib/CodeGen/MacroPPCallbacks.cpp:1
+//===--- MacroPPCallbacks.h -------------------------------------*- C++ -*-===//
+//
----------------
Wrong file header (should be .cpp, and you don't need the "-*- C++ -*-" in a .cpp file.


https://reviews.llvm.org/D16135