So, a couple things:
Firstly:
I think it's pretty sad that this idiom of manually dealing with memory layout
is used all over the place, and has basically no abstraction. In addition to
having to coordinate carefully between allocation and access, as it stands,
it's almost impossible to tell from the header what the layout of the object
actually is -- for example, in Decl.h, there's no hint that ImportDecl has a
SourceLocation array appended to it. This is somewhat unfortunate.
I understand that it's done this way for speed and memory efficiency, but I
can't help but think there *must* be a better way to accomplish it than to
splatter manual object size arithmetic and reinterpret casts so widely over the
codebase.
It'd be great if the GCC zero-length-array extension could be used, or if C++
had adopted C11's flexible array members. That would help in the perhaps 75% of
cases where it's just a single optional trailing object, or array of trailing
objects of the same type.
That makes the allocation size arithmetic easier -- `offsetof(Type,
arrayfield[num_elements])`. It makes the access easier (just reference the
field). And it makes the alignment simply handled by the compiler. I believe
GCC's extension is supported by GCC, Clang, and VC++. I guess there's a desire
to not use extensions like that even if they are supported by the compilers we
care about?
And besides being not standard C++, that also wouldn't work for the ~25% of
cases which have multiple kinds of appended objects, that come one after
another. As an alternative to the GCC zero-length-array option, I had thought
of perhaps making a template base class that handles these sorts of layouts,
somehow, but hadn't really thought it through terribly far how it might work.
(Or if it'd be too complicated to even be worth it.)
Secondly:
There are more cases where the suggestion doesn't work than just forward
declarations. "this + 1" or "X + 1" is only used some of the time, there's many
other ways that code accesses appended objects.
E.g. DeclTemplate.h DependentFunctionTemplateSpecializationInfo has:
FunctionTemplateDecl * const *getTemplates() const {
return
reinterpret_cast<FunctionTemplateDecl*const*>(&getTemplateArgs()[NumArgs]);
}
Or then there's DeclRefExpr
return reinterpret_cast<ASTTemplateKWAndArgsInfo *>(
llvm::alignAddr(&getInternalFoundDecl() + 1,
llvm::alignOf<ASTTemplateKWAndArgsInfo>()));
which re-aligns, and thus doesn't require that getInternalFoundDecl() is
aligned properly for the ASTTemplateKWAndArgsInfo object -- but the base
DeclRefExpr itself really ought to be aligned enough for
ASTTemplateKWAndArgsInfo, or else the layout of the object would change
depending on its address!
Annnnnnnnnnnnnnnnnyways:
I had originally started to put these asserts in the constructors, but that
seemed far from the accesses and was hidden off in a random cpp file, and
sometimes there are multiple of them, spread around. So I tried the accesses,
but sometimes those were spread around multiple places too. So eventually I
just settled on the class definition, because it's always right there and
there's one of it. :)
I can make the requested change, in the subset of cases where it works, and
will certainly do so if still requested. But I'm not really sure it's worth it,
because it removes what is often the only documentation of Weirdness going on,
at the class definition site.
And, I think the Right Thing is to address the deeper problem of what really
seems to me like borderline unreadable code -- somehow. If that can be done, it
will naturally allow alignment asserts to be removed at the same time as the
casts and sizeof arithmetic, because it'd be unnecessary if the compiler or
some common library code was ensuring that allocation and sizing and offsets
and alignment are all done consistently and correctly.
http://reviews.llvm.org/D10272
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
