[PATCH] Emit Clang version information into .comment section (MC's part of implementation) [PART 2B]

[Katya Romanova]

  Regarding a testcase... There is already an existing testcase MC/ELF/ident.s that checks that if .s file contains several .ident directives than the corresponding identifier strings get emitted into .comment section of an ELF object file. I thought it was enough... If not, which tests would you like to add? The ones that report an error when .ident directive is not supported for specific platforms? Or the one that takes .ll file containing llmv.ident metadata, generates .o file and checks that the identifier strings get emitted into .o file?


================
Comment at: lib/MC/MCAsmStreamer.cpp:826
@@ -825,3 +826,2 @@
 
-
 void MCAsmStreamer::EmitFileDirective(StringRef Filename) {
----------------
Rafael Ávila de Espíndola wrote:
> unrelated change.
> 
Fixed

================
Comment at: include/llvm/MC/MCStreamer.h:537
@@ -536,1 +536,3 @@
 
+  /// EmitIdent - Emit the "identifiers" directive.  This implements the
+  /// '.ident "version foo"' assembler directive.
----------------
Rafael Ávila de Espíndola wrote:
> Please don't duplicate the function name.
> 
All the functions in this file have function name duplicated in the comment. As you know, LLVM coding standard says: "If you are extending, enhancing, or bug fixing already implemented code, use the style that is already being used so that the source is uniform and easy to follow." From what I understand, this is considered a "trump card" with respect of other style rules in LLVM. I am a little reluctant not to conform with the style that was being used in MCStreamer.h. Obviously, it won't be hard for me to remove one line from the comment :). Let me know what do you think.

================
Comment at: include/llvm/MC/MCAsmInfo.h:276
@@ +275,3 @@
+    /// true for ELF targets.
+    bool HasIdentDirective;                  // Defaults to true.
+
----------------
Rafael Ávila de Espíndola wrote:
> Why not default to false, that way you only need to set it to true in one place (ELF).
A field just above it, named HasDotTypeDotSizeDirective is being set to true/false in a very similar circumstances. I followed to the same pattern and set HasIdentDirective to true by default. So again, this was more a "style" decision. It won't be hard to change it to the "opposite". Let me know if you want me to change it.

================
Comment at: lib/MC/MCMachOStreamer.cpp:89
@@ +88,3 @@
+  virtual void EmitIdent(StringRef IdentString) {
+    llvm_unreachable("macho doesn't support this directive");
+  }
----------------
Rafael Ávila de Espíndola wrote:
> do we get here if a user tries to use .ident in a .s in OS X? If so, a report_fatal_error is better.
llvm_unreachable was used for consistency with all other unsupported directives in MCMachOStreamer class. If user tries to use .ident directive in .s file in OS X or if he tries to use .size directive, he should get the same error.

Another reason why report_fatal_error was not used, is because report_fatal_error was commented out in EmitFileDirective method.

As with all other comments for this patch, I don't have a strong feeling against doing it one way or another. I just wanted to conform to the existing style that was present the modules where I made modifications. If you want me to change it - let me know.


http://llvm-reviews.chandlerc.com/D1812