[PATCH] D145567: [Driver] Rename multilib flags to tags

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added a comment.

Thanks @MaskRay for taking a look and thanks @simon_tatham for the review of the change. This change affects existing code so I think it deserves its own commit, but I'll move it down the stack to before D142932 <https://reviews.llvm.org/D142932> so that later changes use the new names immediately, and I'll incorporate Simon's suggestions into those.



================
Comment at: clang/docs/Multilib.rst:66
    ``--target=armv7m-none-eabi`` are equivalent. Clang can also accept many
-   independent pieces of information within a single flag - for example
+   independent pieces of information within a single option - for example
    ``-march=armv8.1m.main+fp+mve`` specifies the architecture and two
----------------
simon_tatham wrote:
> An "option" here seems to be the same thing as an "argument" elsewhere in this paragraph. Since the terminology is already confusing, perhaps simplify by using the same word consistently throughout? I think "option" is more precise, because //positional// clang arguments like input files definitely //don't// play a part in this mechanism.
I'll incorporate this into D143587.


================
Comment at: clang/docs/Multilib.rst:71
+   arguments into a standard set of simpler "tags". In many cases these tags
    will look like a command line argument with the leading ``-`` stripped off,
+   but where a suitable form for the tag doesn't exist in command line
----------------
simon_tatham wrote:
> This is a particular case where "option" seems like a better word. Not every //argument// has a leading `-` in the first place. But every //option// does.
> 
> (Or, at least, in the default Unix / gcc style of clang options. I suppose in `clang-cl` even that is not true, because options can have a leading `/` in Windows style. I assume that in that situation the options are normalised to their GNU representation before converting into multilib selection tags?)
I'll incorporate this into D143587.


================
Comment at: clang/docs/Multilib.rst:182
   # List of multilib variants. Required.
   # The ordering of Variants is important if more than one variant can match
+  # the same set of tags. See the docs on multilib layering for more info.
----------------
simon_tatham wrote:
> That capital V looks unintentional to me, and is potentially confusing – someone might go looking for a formal definition of it somewhere.
I'll incorporate this into D143587.


================
Comment at: clang/include/clang/Driver/Multilib.h:64-65
+  /// Get the set of tags that indicate this multilib's use.
+  /// Tags are arbitrary strings although typically they will look similar to
+  /// command line options. A multilib is considered compatible if its tags are
+  /// a subset of the tags derived from the Clang command line options.
----------------
simon_tatham wrote:
> Tags are arbitrary strings, some of which are derived from command-line options and look similar to them, and others can be defined by a particular multilib.yaml
I'll move this change to earlier in the stack before multilib.yaml is a thing, but then I'll update the comment with your suggestion in D142932.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145567/new/

https://reviews.llvm.org/D145567