Details
Diff Detail
Event Timeline
Please add a testcase verifying the interaction between "/arm64EC" and explicitly specifying "--target". (If we end up ignoring the "/arm64EC" flag, we probably want a warning.)
Has Microsoft documented this option? Not doubting it exists but I mostly see how to use it rather than a specific page describing the option and its behaviour.
Not a big deal but if there is one, please include a link to it in the commit message.
| clang/lib/Driver/Driver.cpp | ||
|---|---|---|
| 1384 ↗ | (On Diff #463746) | This would read better to me if you put them all in one and put the most important check first like: if ( UArgs->hasArg(options::OPT__SLASH_arm64EC) &&
( (TC.getTriple().getArch() != llvm::Triple::aarch64 ||
TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec)) { |
| clang/test/Driver/cl-options.c | ||
| 786 | Add an ARM64EC-NOT check for this part to check there is no warning. Not going to catch much in this change but if someone goes digging into target setting they might change it by other means than --target and not realise. | |
| 789 | A space after the ':' is a bit easier to parse IMO (unless this is matching an msvc warning in which case fair enough keep it as is). | |
For now, I haven't found any document for this option. I can update later when Microsoft publish the information.
Please mark comments as done if you feel that they have been addressed. (I know this can be a bit weird, do you mark them or do I, I go with the former, I can disagree if needed)
Lack of docs right now is fine, we have enough to know this exists and it's quite simple.
With my one final comment addressed, this LGTM.
| clang/lib/Driver/Driver.cpp | ||
|---|---|---|
| 1384 ↗ | (On Diff #463746) | This is good but I didn't emphasise one bit. Putting the arm64ec option check first saves reading the rest if the reader knows it's not relevant. |
| clang/lib/Driver/Driver.cpp | ||
|---|---|---|
| 1384 ↗ | (On Diff #463746) | I believe the condition UArgs->hasArg(options::OPT__SLASH_arm64EC) should be much heavier than |
This looks fine as is, all comments addressed as far as I can see.
| clang/lib/Driver/Driver.cpp | ||
|---|---|---|
| 1384 ↗ | (On Diff #463746) | If the cost of the check is higher then this is fine keep it as is. The reason that I personally would reorder them (if costs were equal) is this. Imagine this didn't have 3 conditions it had 100. if ((A or B or C or D or E...
<this continues....>
) && some_option_used)Why did I, the reader, have to parse 99 seemingly random conditions (which have to live in my short term memory) only to find that oh, this is all predicated on that one option being passed. If I don't actually care about that one option, I just wasted my time. And ok, the average if doesn't have 100 conditions but if you keep this in mind even for smaller checks it adds up over time. Code is going to be read more than it is written. This applies to early returns too. Imagine I care about the !=10 case: void foo(int i) {
if (i == 10) {
// 500 lines of code you don't care about 50% of the time
} else {
// what you were actually looking for
return;
}That situation does happen a lot, and the same principle can be applied. void foo(int i) {
if (i != 10)
return;
// 500 lines of code you can read if you actually need to
} |
Thanks for the detail explaination. I agree that if the cost is the same hasArg is better to move to the first.
Based on David's comment I remove the test case with specified target.
Not going to catch much in this change but if someone goes digging into target setting they might change it by other means than --target and not realise.
If you think that's necessary I can commit a NFC change to add it.
I think we want to test both that the warning isn't triggered when --target isn't specified, and that the warning is triggered when --target is specified.
| clang/test/Driver/cl-options.c | ||
|---|---|---|
| 787 | I think the ARM64EC-NOT might be in the wrong place? I think the warning would show up before the final command line, not after it. | |
Add an ARM64EC-NOT check for this part to check there is no warning.
Not going to catch much in this change but if someone goes digging into target setting they might change it by other means than --target and not realise.