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:
> > > > 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 = ...;
> > }
> > ```
> Something like this sufficient?
> ```
> enum ArchPtrSize {Arch16Bit, Arch16Bit, Arch16Bit};
> auto GetArchPtrSize[](llvm::Triple &T) {
> if (T.isArch16Bit())
> return Arch16Bit;
> else if (T.isArch32Bit())
> return Arch32Bit;
> else
> return Arch64Bit;
> };
>
> if (GetArchPtrSize(T) != GetArchPtrSize(TT)) { ... }
>
> ```
1. Make it `static`.
2. `GetArchPtrSize`->`getArchPtrSize`.
3. No need to use `else` if the substatement is return:
```
if (16bit)
return Arch16Bit;
if (32Bit)
return Arch32Bit;
assert(64Bit && "expected 64 bit arch");
return Arch64Bit;
```
4. Move enum to anonymous namespace.
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
