[PATCH] D77394: [Alignment][NFC] Add DebugStr and operator*

Guillaume Chatelet via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Apr 3 08:02:36 PDT 2020


gchatelet marked an inline comment as done.
gchatelet added inline comments.


================
Comment at: llvm/include/llvm/Support/Alignment.h:398
 
+inline Align operator*(Align Lhs, uint64_t Rhs) {
+  assert(Rhs > 0 && "Rhs must be positive");
----------------
courbet wrote:
> courbet wrote:
> > What is the meaning of `Align(1) * 3` ?
> My point is, I think what you want is an `operator<<()`
Since we return an `Align` (or `MaybeAlign`) the result have to be a power of two (or it will assert).
I can make it clear by requesting a power of two for `Rhs`.

I agree that a shift operator has the correct semantic and is less error prone but then the call site would be harder to understand from

```
    MTI->setDestAlignment(I.getDestAlign() * DFSF.DFS.ShadowWidthBytes);
    MTI->setSourceAlignment(I.getSourceAlign() * DFSF.DFS.ShadowWidthBytes);
```
to
```
    MTI->setDestAlignment(I.getDestAlign() << Log2_64(DFSF.DFS.ShadowWidthBytes));
    MTI->setSourceAlignment(I.getSourceAlign() << Log2_64(DFSF.DFS.ShadowWidthBytes));
```

WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77394





More information about the llvm-commits mailing list