rjmccall added inline comments.
================ Comment at: clang/include/clang/AST/Expr.h:2280 } + FPOptionsOverride getFPFeatures(const LangOptions &LO) const { + if (UnaryOperatorBits.HasFPFeatures) ---------------- I would call this one getFPOptionOverrides or getOverriddenFPOptions. If you're going to have a getFPFeatures, it really ought to be the other one — although since we're changing pretty much all of the call sites, we might as well rename it to getFPOptions (or getFPOptionsInEffect, that seems fine, too). ================ Comment at: clang/include/clang/Basic/LangOptions.h:758 + unsigned allow_reciprocal_disabled : 1; + unsigned approx_func_disabled : 1; }; ---------------- As an alternative storage strategy, I would suggest: - in FPOptions, use an integer and masks instead of bit-fields - in FPOptionsOverride, store the same integer from FPOptions plus the active override mask This will make the merge and extract operations really painless. It can get a little boilerplate, though, so I'd recommend also introducing an "x-macro" file: ``` // clang/Basic/FPOptions.def // OPTION(name, type, width, previousName) OPTION(FPContractMode, LangOptions::FPModeKind, 2, First) OPTION(RoundingMode, RoundingMode, 3, FPContractMode) ... OPTION(AllowReciprocal, bool, 1, NoSignedZeros) ... #undef OPTION ... class FPOptions { public: // We start by defining the layout. using storage_type = uint16_t; // Define a fake option named "First" so that we have a PREVIOUS even for the real first option. static constexpr storage_type FirstShift = 0, FirstWidth = 0; #define OPTION(NAME, TYPE, WIDTH, PREVIOUS) \ static constexpr storage_type NAME#Shift = PREVIOUS#Shift + PREVIOUS#Width; \ static constexpr storage_type NAME#Width = width; \ static constexpr storage_type NAME#Mask = ((1 << NAME#Width) - 1) << NAME#Shift; #include "clang/Basic/FPOptions.def" private: storage_type Value; public: // Now you can define constructors and so on. FPOptions() : Value(0) {} bool operator==(FPOptions other) const { return Value == other.Value; } bool operator!=(FPOptionsOverride other) const { return Value != other.Value; } storage_type getAsOpaqueInt() const { return Value; } static FPOptions getFromOpaqueInt(storage_type value) { FPOptions result; result.Value = value; return result; } // We can define most of the accessors automatically: #define OPTION(NAME, TYPE, WIDTH, PREVIOUS) \ TYPE get#NAME() const { \ return TYPE((Value & NAME#Mask) >> NAME#Shift); \ } \ void set#NAME(TYPE value) { \ Value = (Value & ~NAME#Mask) | (storage_type(value) << NAME#Shift); \ } #include "clang/Basic/FPOptions.def" }; // The override type is then basically just that, plus a mask about whether fields are actually set in it: class FPOptionsOverride { FPOptions Options; FPOptions::storage_type OverrideMask = 0; public: FPOptionsOverride() {} bool hasAnyOverrides() const { return OverrideMask != 0; } FPOptions applyOverrides(FPOptions base) { return FPOptions::getFromOpaqueInt(base.getAsOpaqueInt() | (OverrideMask & Options.getAsOpaqueInt())); } bool operator==(FPOptionsOverride other) const { return Options == other.Options && OverrideMask == other.OverrideMask; } bool operator!=(FPOptionsOverride other) const { return !(*this == other); } #define OPTION(NAME, TYPE, WIDTH, PREVIOUS) \ bool has#NAME#Override() const { \ return OverrideMask & NAME#Mask; \ } \ TYPE get#NAME#Override() const { \ assert(has#NAME#Override()); \ return Options.get#NAME(); } \ void clear#NAME#Override() { \ /* Clear the actual value so that we don't have spurious differences when testing equality. */ \ Options.set#NAME(TYPE(0)); \ OverrideMask &= ~NAME#Mask; \ } \ void set#NAME#Override(TYPE value) { \ Options.set#NAME(value); \ OverrideMask |= NAME#Mask; \ } #include "clang/Basic/FPOptions.def" }; ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81869/new/ https://reviews.llvm.org/D81869 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits