[PATCH] D134788: [ARM64EC][clang-cl] Add /arm64EC flag

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

This looks fine as is, all comments addressed as far as I can see.



================
Comment at: clang/lib/Driver/Driver.cpp:1384
+      TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec) {
+    if (UArgs->hasArg(options::OPT__SLASH_arm64EC)) {
+      getDiags().Report(clang::diag::warn_target_override_arm64ec)
----------------
bcl5980 wrote:
> DavidSpickett wrote:
> > DavidSpickett wrote:
> > > 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)) {
> > > ```
> > 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.
> I believe the condition `UArgs->hasArg(options::OPT__SLASH_arm64EC)` should be much heavier than 
> `(TC.getTriple().getArch() != llvm::Triple::aarch64 || TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec)`.
> So I put the Arch and SubArch check first here. I don't understand why we should put the hasArg check first.
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
}
```


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

https://reviews.llvm.org/D134788