[PATCH] D60522: [KnownBits] Add computeForAddCarry()

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
Comment at: llvm/lib/Support/KnownBits.cpp:18
 
-KnownBits KnownBits::computeForAddSub(bool Add, bool NSW,
-                                      const KnownBits &LHS, KnownBits RHS) {
-  // Carry in a 1 for a subtract, rather than 0.
-  bool CarryIn = false;
-  if (!Add) {
-    // Sum = LHS + ~RHS + 1
-    std::swap(RHS.Zero, RHS.One);
-    CarryIn = true;
-  }
+static KnownBits computeForAddCarryInternal(
+    const KnownBits &LHS, const KnownBits &RHS,
----------------
nikic wrote:
> lebedev.ri wrote:
> > The signature is already different from `computeForAddCarry()` (2 `bool`s vs 1 `KnownBits`),
> > i don't see the need for `Internal` suffix.
> > Also, i believe anonymous namespaces are preferred, so drop `static`, and wrap into `namespace {}`.
> Agree on the suffix being unnecessary. Regarding anonymous namespaces, https://llvm.org/docs/CodingStandards.html#anonymous-namespaces says:
> 
> > Because of this, we have a simple guideline: make anonymous namespaces as small as possible, and only use them for class declarations.
> 
> If I'm understanding correctly, this means that anon namespaces should be used for classes and static for functions.
> If I'm understanding correctly, this means that anon namespaces should be used for classes and static for functions.

There have been discussions about exactly that recently, in other reviews.
So i'm not confident what is currently considered as the best common practice for functions.


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

https://reviews.llvm.org/D60522