[PATCH] D81869: Modify FPFeatures to use delta not absolute settings to solve PCH compatibility problems

[John McCall via Phabricator via cfe-commits]

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