[PATCH] D155808: [clang][driver] Missing the condition in IsARMBigEndain function.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:39
 // normalized triple so we must handle the flag here.
 bool arm::isARMBigEndian(const llvm::Triple &Triple, const ArgList &Args) {
+  if ((Triple.getArch() == llvm::Triple::arm ||
----------------
Is this the right place to fix?

I would expect it to be a precondition that the Triple was Arm or Thumb before calling isARMBigEndian?

For example
```
if (Triple.isARM() || Triple.isThumb() || Triple.isAArch64()) {
    bool IsBigEndian = arm::isARMBigEndian(Triple, Args);
    if (IsBigEndian)
      arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
    IsBigEndian = IsBigEndian || Arch == llvm::Triple::aarch64_be;
    CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
  }
```
Shouldn't this be refactored to only call isARMBigEndian for isARM and isThumb? Something like:
```
if ((Triple.isARM() || Triple.isThumb()) {
  bool BigEndian = arm::isARMBigEndian(Triple, Args);
  if (BigEndian)
    arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
  CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
} else if (Triple.isAArch64)) {
  CmdArgs.push_back(Arch == llvm::Triple::aarch64_be ? "-EB" : "-EL");
}
```
This is a bit longer but it is easier to read and keeps ARM and AArch64 separate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155808