[PATCH] D88982: [NFCI] Add LinearPolyBase as base class for ElementCount, TypeSize and StackOffset.

[George Steed via Phabricator via llvm-commits]

georges added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:47-48
+/// The LinearPolyBaseTypeTraits are used to infer the scalar type from the leaf
+/// class.
+template <typename LeafTy, unsigned Dimensions, bool IsUnivariate>
+class LinearPolyBase {
----------------
Not a suggestion, but just an observation that we're passing some arguments to the class via template parameters and others via the traits object. Seems inconsistent?


================
Comment at: llvm/include/llvm/Support/TypeSize.h:56-57
 
-  static constexpr PolySize getFixed(T MinVal) { return {MinVal, false}; }
-  static constexpr PolySize getScalable(T MinVal) { return {MinVal, true}; }
-  static constexpr PolySize get(T MinVal, bool IsScalable) {
-    return {MinVal, IsScalable};
+protected:
+  std::array<ScalarTy, Dimensions> Coefficients;
+  unsigned UnivariateDim;
----------------
Is it worth specializing this struct template for IsUnivariate=true/false, since with the number of enable_if's scattered around they begin to look like quite different classes? It also seems a bit wasteful to be allocating Dimensions elements when all but one of them will be zero in the Univariate case?


================
Comment at: llvm/include/llvm/Support/TypeSize.h:118
+  template <typename U = ScalarTy>
+  friend typename std::enable_if<std::is_signed<U>::value, LeafTy>::type
+  operator-(const LeafTy &LHS) {
----------------
nit: I think you can use the _v/_t versions of these functions here and elsewhere since we're in C++14 now, e.g. `std::enable_if_t<std::is_signed_v<U>, LeafTy>`.


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

https://reviews.llvm.org/D88982