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

[Rafael Ávila de Espíndola]

================
Comment at: include/llvm/MC/MCAsmInfo.h:276
@@ +275,3 @@
+    /// true for ELF targets.
+    bool HasIdentDirective;                  // Defaults to true.
+
----------------
Katya Romanova wrote:
> 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.
Please do. There is a balance between following the code that is already there and improving the status quo. For simple things like this it is probably better to use the default value that makes the most sense for this particular field.


================
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.
----------------
Katya Romanova wrote:
> 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.
Same as above: it is a balance. Since the duplicated name adds not value and is not a big difference, I would go with "remove it".

================
Comment at: include/llvm/MC/MCStreamer.h:539
@@ +538,3 @@
+  /// '.ident "version foo"' assembler directive.
+  virtual void EmitIdent(StringRef IdentString) = 0;
+
----------------
What about giving this a default implementation that does nothing? Would save on some duplicated code. ]


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