This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Separate getLastArgIntValue to Basic
ClosedPublic

Authored by yaxunl on Dec 5 2019, 10:58 AM.

Details

Summary

getLastArgIntValue is a useful utility function to get command line argument as an integer.
Currently it is in Frontend so that it can only be used by clang -cc1. Move it to basic so
that it can also be used by clang driver.

Diff Detail

Event Timeline

yaxunl created this revision.Dec 5 2019, 10:58 AM
tra added inline comments.Dec 5 2019, 11:52 AM
clang/lib/Basic/OptionUtils.cpp
19

I'd use anonymous namespace instead of static.

23

Does it have to be base-10?
Now that the functions are part of Basig API intended for wider use, I'd argue for making it more flexible by default. If specific base is needed, then we could add an argument to specify it.

34

This needs updating.

yaxunl updated this revision to Diff 232592.Dec 6 2019, 9:40 AM
yaxunl marked 4 inline comments as done.

revised by Artem's comments.

clang/lib/Basic/OptionUtils.cpp
19

will do

23

Added

34

fixed

tra added inline comments.Dec 6 2019, 10:45 AM
clang/include/clang/Basic/OptionUtils.h
24

What are the rules on using LLVM headers here? Can we just include llvm/Option/ArgList.h instead?

46

Same question as before -- does it have to be 10?
0 would be a more reasonable default for general use. IMO we care about the value, but not so much about the form. I.e. is there a reason not to allow 0xf, for instance, if that works for the user?

clang/lib/Basic/CMakeLists.txt
1

I think now that you're using ArgList, you need to depend on LLVM's LLVMOption library.
As is you're likely to run into build issues if shared libraries are enabled.

yaxunl marked 5 inline comments as done.Dec 6 2019, 1:16 PM
yaxunl added inline comments.
clang/include/clang/Basic/OptionUtils.h
24

In the API llvm::opt::ArgList is used as reference, therefore it only needs a forward declaration. llvm::opt::OptSpecifier is passed by value, therefore it needs header. OptSpecifier is a small class, therefore it is not efficient to pass by reference.

46

will do. StringRef will do radix autosensing for that.

clang/lib/Basic/CMakeLists.txt
1

will do

yaxunl updated this revision to Diff 232639.Dec 6 2019, 1:28 PM
yaxunl marked 2 inline comments as done.

revised by Artem's comments

tra accepted this revision.Dec 19 2019, 2:07 PM
This revision is now accepted and ready to land.Dec 19 2019, 2:07 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2019, 5:40 PM
thakis added a subscriber: thakis.Dec 21 2019, 6:06 PM

Why move this to Basic instead of to Driver if you want to use it in Driver? (Frontend depends on Driver so it could then still use it.)

Why move this to Basic instead of to Driver if you want to use it in Driver? (Frontend depends on Driver so it could then still use it.)

You are right. Driver is a better place. I will create a review for it.

(The follow-up is at D71802. Thanks!)