aprantl added a comment. Thanks! Couple more inline comments.
================ Comment at: lib/CodeGen/CGDebugInfo.h:415 + /// Get macro debug info. + llvm::DIMacro *CreateMacro(llvm::DIMacroFile *Parent, unsigned MType, ---------------- It would be good to be a bit more descriptive here. If the comment just repeats the name of the function it is not worth keeping, /// Create debug info for a macro (definition/use/?). ================ Comment at: lib/CodeGen/CGDebugInfo.h:420 + + /// Get temporary macro file debug info. + llvm::DIMacroFile *CreateTempMacroFile(llvm::DIMacroFile *Parent, ---------------- E.g., /// Create debug info for a file referenced by an #include directive. ================ Comment at: lib/CodeGen/MacroPPCallbacks.cpp:44 + if (MI.isGNUVarargs()) + Name << "..."; // #define foo(x...) + ---------------- aaboud wrote: > 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. I think it would be a good idea to clean this up. Feel free to either do this now or in a separate commit. ================ Comment at: lib/CodeGen/MacroPPCallbacks.cpp:154 + StringRef SearchPath, StringRef RelativePath, const Module *Imported) { + // Only care about "include" directives. + if (!IncludeTok.is(tok::identifier)) ---------------- aaboud wrote: > 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. #import comes from Objective-C. It is equivalent to #include + #pragma once. I'm curious whether this comment means that it isn't handled? ================ Comment at: lib/CodeGen/MacroPPCallbacks.cpp:88 + // Invalid location represents line zero. + return SourceLocation(); +} ---------------- I think the entire function can be replaced with ``` if ((Status == MainFileScope) || (Status == CommandLineScopeIncludes && CommandIncludeFiles)) return Loc; return SourceLocation(); ``` ================ Comment at: lib/CodeGen/MacroPPCallbacks.cpp:100 + bool CreateMacroFile = false; + bool PopScope = false; + SourceLocation LineLoc = getCorrectLocation(LastHashLoc); ---------------- Could you try splitting this function up into two functions? MacroPPCallbacks::FileEntered() MacroPPCallbacks::FileExited() and the common code factored out into a helper? The control flow here seems unnecessarily complex with all the flags. ================ Comment at: lib/CodeGen/MacroPPCallbacks.cpp:177 + llvm::raw_string_ostream Value(ValueBuffer); + calculatMacroDefinition(*Id, *MD->getMacroInfo(), PP, Name, Value); + Gen->getCGDebugInfo()->CreateMacro(getCurrentScope(), ---------------- calculat*e*MacroDefinition (is there a better word than calculate?) https://reviews.llvm.org/D16135 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits