This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Rename multilib flags to tags
AbandonedPublic

Authored by michaelplatings on Mar 8 2023, 3:10 AM.

Details

Summary

The name "flag" implies a type of command line option which is no longer
how these variables are used in multilib. Although tags often look
similar to command line options this is not a requirement - tags may be
arbitrary strings.

MultilibBuilder still uses the name "flag" since it is designed to be
used only with valid command line options, even though these are
subsequently used as multilib tags. Similarly addMultilibFlag() keeps
its existing name because of how it is used, even though it now takes a
tag_set argument.

Diff Detail

Unit TestsFailed

Event Timeline

michaelplatings created this revision.Mar 8 2023, 3:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 3:10 AM
Herald added a subscriber: abrachet. · View Herald Transcript
michaelplatings requested review of this revision.Mar 8 2023, 3:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 3:10 AM

Since the touched code is still in review, you need to merge the changes into the prior patches.

MaskRay requested changes to this revision.Mar 8 2023, 4:10 PM
This revision now requires changes to proceed.Mar 8 2023, 4:10 PM

But it was useful to have it separate at least for review purposes, since it made it much easier when Michael asked me to proofread the change from 'flags' to 'tags'!

clang/docs/Multilib.rst
66 ↗(On Diff #503293)

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.

182 ↗(On Diff #503293)

That capital V looks unintentional to me, and is potentially confusing – someone might go looking for a formal definition of it somewhere.

clang/include/clang/Driver/Multilib.h
63–64

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

simon_tatham added inline comments.Mar 9 2023, 2:59 AM
clang/docs/Multilib.rst
71 ↗(On Diff #503293)

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?)

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 so that later changes use the new names immediately, and I'll incorporate Simon's suggestions into those.

clang/docs/Multilib.rst
66 ↗(On Diff #503293)

I'll incorporate this into D143587.

71 ↗(On Diff #503293)

I'll incorporate this into D143587.

182 ↗(On Diff #503293)

I'll incorporate this into D143587.

clang/include/clang/Driver/Multilib.h
63–64

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.

michaelplatings marked an inline comment as done.Mar 9 2023, 1:09 PM

Tiny tweak: undo an unnecessary change to a test

peter.smith added a subscriber: peter.smith.

I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions.
From looking at the source code alone - assuming that most people would not track down the commit message/review for extra context - I found it difficult to work out the convention for when Flags is used and when Tags is used. I've made a suggestion for some comments. This can be applied prior to commit if you want to take it up.

clang/lib/Driver/ToolChains/CommonArgs.h
202

I can see the reason to keep the name addMultilibFlag. At this point is the tag_set expected to be simplified tags or full command line flags. If it is the former I think it would be good to change Flags to Tags here.

May also be useul to add a \p for Flags (or Tags) if there are any requirements, or just useful information on what it is expected to be.

Parameter name also applies to CommonArgs.cpp below.

peter.smith accepted this revision.Mar 13 2023, 9:22 AM

Actually click the button this time to set approval, see previous comment.

michaelplatings marked an inline comment as done.Mar 13 2023, 10:49 AM

Thanks @peter.smith. I've opted to leave the comment as-is. If we can expect a tag_set to actually contain flags then I've continued to use that terminology.

clang/lib/Driver/ToolChains/CommonArgs.h
202

For the way this function is used I would expect Flags to be command line flags prefixed with + or -, the same as for the Flag parameter. Therefore I think it best to leave it as-is.

michaelplatings abandoned this revision.Mar 14 2023, 2:38 AM
michaelplatings marked an inline comment as done.