[PATCH] D68340: Add AIX toolchain and basic linker functionality

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:35
+  // Only support 32 and 64 bit
+  if (!IsArch32Bit && !IsArch64Bit)
+    llvm_unreachable("Unsupported bit width value");
----------------
stevewan wrote:
> jasonliu wrote:
> > Xiangling_L wrote:
> > > Is there any reason to use llvm_unreachable here? I think we should use  'assertion' instead here:
> > > 
> > > ```
> > > assert((IsArch32Bit || IsArch64Bit) && "...");
> > > ```
> > IsArch64Bit used only in the assertion could cause warning when the assertion is turned off. 
> Jason has provided a good point why `llvm_unreachable` was preferred here. Other than that, I believe the two are fairly interchangeable in this particular case. That said, I'm leaning towards keeping `llvm_unreachable`, but definitely add more comment if you have good reasons for using `assert`. Thanks!
Jason is right, I am fine with keeping `llvm_unreachable`.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:44
+  } else {
+    assert(Output.isNothing() && "Invalid output.");
+  }
----------------
Glad to know the build without assertion on would not be affected by this. I just have slight preference that we don't have this blank block in our product code when the assertion is off. Is that better we put this assertion before `if` block, and do something like this;

```
assert((Output.isFilename() || Output.isNothing()) && "Invalid output.");

if (Output.isFilename()) {
    CmdArgs.push_back("-o");
    CmdArgs.push_back(Output.getFilename());
  } 
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68340