aaboud added a comment. Thanks Adrian for the fast response. See some answers below.
I will update the code and upload a new patch soon. ================ 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: > 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? I will remove the switch and update the LastHashLoc unconditionally. I did not check Objective-C debug info, but I assume it will work fine once I change these lines. ================ Comment at: lib/CodeGen/MacroPPCallbacks.cpp:88 + // Invalid location represents line zero. + return SourceLocation(); +} ---------------- aprantl wrote: > I think the entire function can be replaced with > ``` > if ((Status == MainFileScope) || > (Status == CommandLineScopeIncludes && CommandIncludeFiles)) > return Loc; > return SourceLocation(); > ``` Sure, will do. The reason I have a switch, because I had more cases during the implementation that I get rid of. ================ Comment at: lib/CodeGen/MacroPPCallbacks.cpp:100 + bool CreateMacroFile = false; + bool PopScope = false; + SourceLocation LineLoc = getCorrectLocation(LastHashLoc); ---------------- aprantl wrote: > 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. Will try restructure. The way I implemented this is by having stages (parsing levels). The control flow is to identify the current stage and decide how to continue from there. I will see what I can do to make the code much clear. ================ Comment at: lib/CodeGen/MacroPPCallbacks.cpp:177 + llvm::raw_string_ostream Value(ValueBuffer); + calculatMacroDefinition(*Id, *MD->getMacroInfo(), PP, Name, Value); + Gen->getCGDebugInfo()->CreateMacro(getCurrentScope(), ---------------- aprantl wrote: > calculat*e*MacroDefinition > (is there a better word than calculate?) How about: GenerateMacroDefinition PrintMacroDefinition ParseMacroDefinition Do you think any of the above would be better choice? https://reviews.llvm.org/D16135 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits