[PATCH] D110900: Triple: Add RedHat vendor

[Aaron Puchert via Phabricator via llvm-commits]

aaronpuchert added inline comments.


================
Comment at: llvm/include/llvm/ADT/Triple.h:532
   bool isGNUEnvironment() const {
     EnvironmentType Env = getEnvironment();
     return Env == Triple::GNU || Env == Triple::GNUABIN32 ||
----------------
Since `getEnvironment` is also public, shouldn't we fix it there? Otherwise I fear this might lead to inconsistencies. (That is, `getEnvironment()` returning `UnknownEnvironment`, but `isGNUEnvironment()` returning `true`.) Perhaps we should even do something like this:

```
  bool isGNUEnvironment() const {
    return isGNU(getEnvironment());
  }
  static bool isGNU(EnvironmentType Env) {
    // ...
  }
```

to clarify that `isGNUEnvironment` really only depends on the environment.

Possibly we should even set `Environment = Triple::GNU;` in the constructor for vendors known to use GNU by default without declaring it?

That's currently done in some cases, though I'm not sure if the members should contain the properties as declared, or the actual values.


================
Comment at: llvm/include/llvm/ADT/Triple.h:536
+           Env == Triple::GNUEABIHF || Env == Triple::GNUX32 ||
+           (getVendor() == Triple::RedHat && Env == Triple::UnknownEnvironment);
   }
----------------
The `Triple::` qualification should not be needed inside the class. Though it's used throughout the class, so maybe we should address that separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110900