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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
