[PATCH] D140263: [NFC] Vastly simplifies TypeSize

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:168
 
-  /// Add \p RHS to the value at the univariate dimension.
-  LeafTy getWithIncrement(ScalarTy RHS) const {
-    return static_cast<LeafTy>(
-        UnivariateLinearPolyBase(Value + RHS, UnivariateDim));
+  /// Add \p Rhs to the value at the univariate dimension.
+  LeafTy getWithIncrement(ScalarTy Rhs) const {
----------------
gchatelet wrote:
> gchatelet wrote:
> > barannikov88 wrote:
> > > gchatelet wrote:
> > > > barannikov88 wrote:
> > > > > univariate dimension --> fixed quantity?
> > > > > 
> > > > Reworded it MinValue since this is what it is.
> > > Thanks. However, MinValue is an implementation detail that should not be exposed even in comments.
> > > Thanks. However, MinValue is an implementation detail that should not be exposed even in comments.
> > 
> > Is that true ? I'm under the impression that this is what it was intended for in the original patch
> > https://github.com/llvm/llvm-project/commit/b302561b763a1d2eb1a450e135b8d49931936755
> > 
> > ```
> > * Adds a TypeSize struct to represent the known minimum size of a type
> >   along with a flag to indicate that the runtime size is a integer multiple
> >   of that size
> > ```
> Let me rephrase "yes you're right that this is an implementation detail and it shouldn't be exposed but in that particular case it seems to be the exact semantic for the type".
> I think it's better to update the documentation to reflect what was in the original comment. WDYT?
Sorry for not being clear clear.
I meant that "MinValue" (spelled exactly) is a private field, which users of the class should not care about. "Minimum value", e.g., is different.
Since this class is now abstract, I'd avoid using "Min" in variable name and "Minimum" in the comment.
Finally, I'd remove this method and add `operator+` instead :) There is an (outdated?) comment near divideCoefficientBy that suggests of not doing it though.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140263