[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud marked 7 inline comments as done.
aaboud added a comment.

Thanks Adrian and Richard for the comments, I appreciate that.
I am uploading a new patch that address most of the comments.

Adrian, I answered some of your comments below.

Thanks,
Amjad



================
Comment at: lib/CodeGen/MacroPPCallbacks.cpp:31
+      MacroInfo::arg_iterator AI = MI.arg_begin(), E = MI.arg_end();
+      for (; AI + 1 != E; ++AI) {
+        Name << (*AI)->getName();
----------------
aprantl wrote:
> std::next() ?
I am not that satisfied with this function, and I hope we can do it in a better way.
This function is based on a code I took from this a static function in file "lib\Frontend\PrintPreprocessedOutput.cpp":

```
/// PrintMacroDefinition - Print a macro definition in a form that will be
/// properly accepted back as a definition.
static void PrintMacroDefinition(const IdentifierInfo &II, const MacroInfo &MI,
                                 Preprocessor &PP, raw_ostream &OS) {
```


================
Comment at: lib/CodeGen/MacroPPCallbacks.cpp:44
+    if (MI.isGNUVarargs())
+      Name << "..."; // #define foo(x...)
+
----------------
aprantl wrote:
> LLVM style prefers comments to be full sentences and above the code.
As I said above, this code is taken from "lib\Frontend\PrintPreprocessedOutput.cpp"
Also, I ran clang-format on it, and it did not suggest to change this line.

But, I do not mind fixing this, if it is the preferred comment style.


================
Comment at: lib/CodeGen/MacroPPCallbacks.cpp:154
+    StringRef SearchPath, StringRef RelativePath, const Module *Imported) {
+  // Only care about "include" directives.
+  if (!IncludeTok.is(tok::identifier))
----------------
aprantl wrote:
> #include as opposed to #import? If so, why?
I am not familiar with "#import" keyword.
1. Is it handled differently by clang that #include?
2. Do we need to generate macro debug info for it?

The reason I have this switch below, is that I am not sure if we call this "InclusionDirective" callback for identifiers that are not "#include".

However, I see no harm of updating the LastHashLoc every time we enter this function. The value will not be used unless it is set just before calling "FileChanged"callback.


================
Comment at: lib/CodeGen/ModuleBuilder.cpp:68
     SmallVector<CXXMethodDecl *, 8> DeferredInlineMethodDefinitions;
+    CGDebugInfo *DebugInfoRef;
 
----------------
aprantl wrote:
> What does Ref stand for?
Ref - stands for reference, I think I do not need the "Ref" prefix for this member, only for the get function.
But, I will follow Richards suggestion that will eliminate the need for this member.


https://reviews.llvm.org/D16135