On 30 September 2013 11:28, Katya Romanova <[email protected]> wrote: > > Hi Rafael, > > Thank you so much for the review. I'm not sure I completely understand your > question > "Do you expect llvm to reason about this in any way?". If you mean whether > llvm will use this information in any way, than the answer is "yes". Please > refer to the Phabricator review http://llvm-reviews.chandlerc.com/D1729 "Emit > Clang version information into .comment section (LLVM's part of > implementation) [PART 2]" for information on how llvm is consuming the > produced metadata. Also, in that review I discuss why emitting Clang's > version info through inline asm with .ident is not such a good solution as > emitting it as metadata. If you don't have time to read that review, let me > briefly describe the reasons: > > (1) llvm.ident metadata could be used not only by LLVM, but by other > consumers that read LLVM IR > and want to generate a version information. It is easier for the LLVM IR > consumers to understand the metadata than parse and understand inlined > assembly. > (2) llvm.ident metadata could be produced not only by Clang, but by other > producers that generate LLVM IR. Again, producing the metadata is easier then > producing inlined assembly. > (3) It makes it easier to handle non-elf targets that don't support .ident.
Yes, sorry I missed that thread before reading this one. I think I agree with the argument that it makes it easier to check where an IL file comes from. I was thinking only of the final object file. > With respect of the changes that you suggested. > - I modified the testcase to handle cases where CLANG_VENDOR is defined. > - I'm hesitant to spell EmitVersionIdentMetadata() function name with > lowercase. All other functions > Emit<Something> in CodeGenModule.h and CodeGenModule.cpp are spelled with > the first uppercase letter. > That's the reason why I did the same. Otherwise, my function will really > stand out among > all other functions in these files. I think it better to do things > consistently and commit "as is", but at some point of time to refactor and > change the spelling of *all* of the function names in these files with > lowercase. > Let me know if you are OK with keeping the name of > EmitVersionIdentMetadata() spelled with uppercase first letter. Yes, that is fine. My preference is to use the new style guide, but the line for consistency with old code and the new style is fuzzy. > I will send an updated patch when I hear back from you. > > http://llvm-reviews.chandlerc.com/D1720 Cheers, Rafael _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
