jansvoboda11 requested review of this revision.
jansvoboda11 added a comment.
I investigated this a bit more.
What previously lead me to believe that `SourceLoc::ID` is being modified from
the outside was the way it's declared right after some `friend`s:
class SourceLocation {
friend class ASTReader;
friend class ASTWriter;
friend class SourceManager;
friend struct llvm::FoldingSetTrait<SourceLocation>;
public:
using UIntTy = uint32_t;
using IntTy = int32_t;
private:
UIntTy ID = 0;
I looked up the actual writes to `ID` and they only occur in the
`SourceLocation` factory functions, for example:
SourceLocation getLocWithOffset(IntTy Offset) const {
assert(((getOffset()+Offset) & MacroIDBit) == 0 && "offset overflow");
SourceLocation L;
L.ID = ID+Offset;
return L;
}
Theoretically there's nothing preventing the friends from modifying the field
as well, but that's not being abused at this moment.
This means that if the `FullSourceLoc(SourceLocation, SourceManager &)`
constructor was to be called with an invalid `SourceLocation`, it would stay
invalid for the whole lifetime of `FullSourceLoc`.
---
So if we really expect <https://reviews.llvm.org/D31709?id=95541#inline-279799>
`FullSourceLoc` to always uphold this invariant: `(SrcMgr != nullptr) ==
isValid()`, we should be able to replace this assertion:
bool hasManager() const {
bool hasSrcMgr = SrcMgr != nullptr;
assert(hasSrcMgr == isValid() && "FullSourceLoc has location but no
manager");
return hasSrcMgr;
}
with this one:
explicit FullSourceLoc(SourceLocation Loc, const SourceManager &SM)
: SourceLocation(Loc), SrcMgr(&SM) {
assert(isValid() && "FullSourceLoc has manager but no location");
}
That way, we can catch any future misuse of `FullSourceLoc` much earlier.
---
But: I think the intention for `FullSourceLoc` has never been to uphold that
invariant:
- `FullSourceLoc` is being constructed without clients ensuring the validity of
the `SourceLoc`.
- Moving the assertion into the constructor and running tests confirmed
`FullSourceLoc` is indeed being constructed with invalid `SourceLocation` (and
valid `SourceManager`).
- The clients also never check `isValid()` before calling `hasManager()` to
avoid the assertion in `hasManager()`, which is odd.
- I think the assertion in `hasManager` has never been hit before simply by
pure luck.
---
Considering that `FullSourceLoc` has been around since 2007 and the assertion
was just added in 2017, I think the correct thing to do here is to remove the
assertion and properly document that `FullSourceLoc` does not uphold any
invariants.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106862/new/
https://reviews.llvm.org/D106862
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
