[PATCH] D97204: [RFC] Clang 64-bit source locations

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added a comment.

Hmmm. Two people have pointed out to me that my strategy of having a 32-bit `SourceLocations::LowBits` and an 0- or 32-bit `SourceLocations::OptionalHighBits` doesn't actually work, because an empty struct still takes at least 1 byte. So this version of the patch will still increase memory usage in the 32SL configuration, which is just what I was trying to avoid. Whoops.

@rsmith, do you have any thoughts on what would be an acceptable replacement strategy? You mentioned wanting to avoid actual #ifdefs all over the AST class hierarchy. Some possible alternatives:

We could define a family of macros: one for declaring a possible-SourceLocation in a branch of the Stmt bitfields union, one for declaring the same SourceLocation in a particular Stmt subclass, and a pair for the accessor functions that read and write whichever of those exists. Then there'd be slightly ugly macro calls all over Stmt.h and friends, but only one outright #ifdef, where the macros are defined.

(And then there are two options for how to define the macros in the 64SL case: either move the whole SourceLocation into the subclasses for speed, or keep half of it in the bitfields as now, for space. But that decision could be localized into the macro definitions, and easily changed.)

Alternatively, @miyuki suggests that my SourceLocation::OptionalHighBits could become an extra base class of some of the Stmt subclasses, so that empty base optimization //would// allow it to take up 0 bytes in the 32SL configuration.

The macro approach is the kind of thing I wouldn't mind doing in my own projects, but then, I have a strong stomach for macros :-) and I'd rather check before I do all the work to rewrite this patch in one of those styles, only to find out that you'd prefer another, or something I haven't even thought of.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97204