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

[Rafael Espíndola]

>   Right now, we support multiple metadata nodes in llvm.ident, where each of these metadata nodes could have a list of strings:
>   +!0 = metadata !{metadata !"clang version x.x"}
>   +!1 = metadata !{metadata !"something else", metadata !"plus another"}
>   +!2 = metadata !{i32 1}
>   ----------------
>
>   I believe you didn't like the fact that each of the metadata nodes could have a list of strings (i.e. you didn't like the following):
>   +!1 = metadata !{metadata !"something else", metadata !"plus another"}

I don't like having multiple ways of doing it.

>   Are you OK with having multiple metadata nodes in llvm.ident?
>   +!0 = metadata !{metadata !"clang version x.x"}
>   +!1 = metadata !{metadata !"something else"}
>   +!2 = metadata !{i32 1}
>
>   Or do you want only one metadata entry with just one string in it?
>   +!0 = metadata !{metadata !"clang version x.x"}

I am OK with either
* llvm.ident takes a list of metadata entry. Each entry has just one string.
* llvm.ident tokes a single metadata entry. That entry has multiple stings.

With a small preference for the first one.

>   Is there any particular reason why would you want impose a limitation on the structure of metadata node in a llvm.ident? ln the future llvm.ident could be used for many different reasons, not only for storing compiler version information. For example, it could be used when  #ident directive is implemented or if we decide to emit compilation information (like a list of compilation options) to .comment section, etc.

Just the "there should be just one way to do it". This can always be
extended in the future, but for now it should be as simple as possible
for what we need.

>   I don't see many justifications for limiting the structure of metadata node in llvm.ident. Why do you want to do that? The implementation of AsmPrinter::EMitModuleIdents() came out naturally with multiple strings in a metadata node. It will be more effort to limit this (i.e. allow only one string in a metadata node), because of all the checks and validations.
>
>   I'm definitely against making it to be just one metadata node with just one string in it +!0 = metadata !{metadata !"clang version x.x"}. It will make it much harder to add something to llvm.ident if we want to. Hopefully, you don't want to impose this limitation.

I am ok with being able to represent mulitple ident lines. But there
should be only one way to represent it.

>   Also, could you please review and make comments on MC part of the patch? I have to switch back and forth between different non-compilable pieces of the patch now (since the patch is split). It's time consuming and error prone. It will be easier to make all the corrections, test and then send updated reviews for both parts.

Please upload to an independent review. Two patches at one phabricator
review is somewhat annoying. It looks the same as an old version of
the same patch.

>   P.S. Is Clang's part of the patch OK?

I think it is, but please wait for the llvm ones to be checked in first.

Cheers,
Rafael