[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via cfe-commits]

aaboud added a comment.

Thanks Richard for reviewing the patch and for the comments.
Please, see answers below.

Regards,
Amjad



================
Comment at: include/clang/AST/ASTConsumer.h:163
+  /// The caller takes ownership on the returned pointer.
+  virtual std::unique_ptr<PPCallbacks> CreatePreprocessorCallbacks(Preprocessor &PP);
 };
----------------
rsmith wrote:
> aaboud wrote:
> > Richard,
> > I know that you suggested not to introduce the Preprocessor in anyway to the ASTConsumer.
> > However, as I could not understand the other way to support macros in Clang without applying a huge redesign, I decided to upload the code I suggested in the proposal.
> > http://lists.llvm.org/pipermail/llvm-dev/2015-November/092449.html
> > 
> > If you still think that this approach is not the right one, I would appreciate it if you can explain again how to implement the macro support differently.
> Don't add this to `ASTConsumer`; an AST consumer and a PP consumer are two largely separate ideas. Instead, you could create and register a `PPCallbacks` object directly from `CodeGenAction::CreateASTConsumer`. In fact, if you look there, a `PPCallbacks` subclass is already registered (for code coverage); you can do something similar to register your `MacroPPCallbacks` object.
Thanks, I will try this approach, it is indeed cleaner.


================
Comment at: lib/CodeGen/CMakeLists.txt:76
   ItaniumCXXABI.cpp
+  MacroPPCallbacks.cpp
   MicrosoftCXXABI.cpp
----------------
rsmith wrote:
> This file is missing from the diff.
Sorry about that, it seems when I updated the code these files were removed from the patch.
Also, I need to improve the implementation in these two files.

By the way, you can find an old version of them in the previous diff.

I will update them and upload them to the review together with the above suggestion of yours.


https://reviews.llvm.org/D16135