ABataev added inline comments.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3209-3214
+ else if ((T.isArch64Bit() && TT.isArch32Bit()) ||
+ (T.isArch64Bit() && TT.isArch16Bit()) ||
+ (T.isArch32Bit() && TT.isArch64Bit()) ||
+ (T.isArch32Bit() && TT.isArch16Bit()) ||
+ (T.isArch16Bit() && TT.isArch32Bit()) ||
+ (T.isArch16Bit() && TT.isArch64Bit()))
----------------
jhuber6 wrote:
> ABataev wrote:
> > jhuber6 wrote:
> > > ABataev wrote:
> > > > Maybe do something like `Triple::getArchPointerBitWidth(T.getArch()) !=
> > > > Triple::getArchPointerBitWidth(TT.getArch())"?
> > > That was my first thought but that function is private to the Triple
> > > class so I just went with this brute force method. The file has this
> > > comment for the justification.
> > >
> > >
> > > ```
> > > /// Test whether the architecture is 64-bit
> > > ///
> > > /// Note that this tests for 64-bit pointer width, and nothing else.
> > > Note
> > > /// that we intentionally expose only three predicates, 64-bit,
> > > 32-bit, and
> > > /// 16-bit. The inner details of pointer width for particular
> > > architectures
> > > /// is not summed up in the triple, and so only a coarse grained
> > > predicate
> > > /// system is provided.
> > > ```
> > Would it be better to convert the results of `isArch...Bit()` to enum and
> > compare enums then? Like:
> > ```
> > enum {Arch16bit, Arch32bit, Arch64bit} TArch, TTArch;
> >
> > if (T.isArch16Bit())
> > Tarch = Arch16Bit;
> > else if ...
> > ...
> > // Same for TT.
> > if (TArch != TTArch)
> > ...
> > ```
> You'd end up with a similar situation right, having about 6 conditionals
> manually checking check predicate right?
>
> ```
> if (T.isArch16Bit())
> else if (T.isArch32Bit())
> else if (T.isArch64Bit())
> if (TT.isArch16Bit())
> else if (TT.isArch32Bit())
> else if (TT.isArch64Bit())
> ```
Yes, but it may be easier to maintain in the future, say when 128 or 256 bits
architectures are added? :)
Plus, the last one should not be `else if`, just:
```
else {
assert(T.isArch64Bit() && "Expected 64 bits arch");
TArch = ...;
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88594/new/
https://reviews.llvm.org/D88594
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
