Page MenuHomePhabricator

[Basic] Introduce PODSourceLocation, NFCI
Needs RevisionPublic

Authored by miyuki on Nov 6 2019, 9:19 AM.

Details

Summary

The goal of this change is to further reduce the number of cases when
the internal representation of SourceLocation is accessed directly.

The change adds a new class PODSourceLocation and replaces 'unsigned'
with 'PODSourceLocation' where possible. The PODSourceLocation class
holds the same information as SourceLocation, but it is a POD type,
hence it can be used in contexts where SourceLocation cannot be used
(e.g. unions with implicit default constructors).

SourceLocation objects can be converted to PODSourceLocation and
constructed from PODSourceLocation using the corresponding helpers
getPOD and getFromPOD.

Diff Detail

Event Timeline

miyuki created this revision.Nov 6 2019, 9:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2019, 9:20 AM
dexonsmith requested changes to this revision.Oct 19 2020, 2:36 PM
dexonsmith added a subscriber: dexonsmith.

An alternative would be to update the unions to an AlignedCharArrayUnion and use SourceLocation directly. WDYT?

clang/include/clang/AST/DeclObjC.h
652–655

IIRC, this will not be a POD, since the access of the members of PODSourceRange is public and the access of the members of PODSourceLocation is private.

This revision now requires changes to proceed.Oct 19 2020, 2:36 PM

An alternative would be to update the unions to an AlignedCharArrayUnion and use SourceLocation directly. WDYT?

So, say, in DeclarationNameLoc, I would add AlignedCharArrayUnion as follows:

union {
  llvm::AlignedCharArrayUnion<struct NT, struct CXXOpName, struct CXXLitOpName> UninitStorage;
  struct NT NamedType;
  struct CXXOpName CXXOperatorName;
  struct CXXLitOpName CXXLiteralOperatorName;
};

And change the constructor of DeclarationNameLoc to default-construct UninitStorage, i.e.:

DeclarationNameLoc() : UninitStorage() {
  memset(UninitStorage.buffer, 0, sizeof(UninitStorage.buffer));
}

After that, I can use SourceLocation in DeclarationNameLoc directly.

Do I understand your idea correctly?

An alternative would be to update the unions to an AlignedCharArrayUnion and use SourceLocation directly. WDYT?

So, say, in DeclarationNameLoc, I would add AlignedCharArrayUnion as follows:

union {
  llvm::AlignedCharArrayUnion<struct NT, struct CXXOpName, struct CXXLitOpName> UninitStorage;
  struct NT NamedType;
  struct CXXOpName CXXOperatorName;
  struct CXXLitOpName CXXLiteralOperatorName;
};

And change the constructor of DeclarationNameLoc to default-construct UninitStorage, i.e.:

DeclarationNameLoc() : UninitStorage() {
  memset(UninitStorage.buffer, 0, sizeof(UninitStorage.buffer));
}

After that, I can use SourceLocation in DeclarationNameLoc directly.

Do I understand your idea correctly?

Not quite, I wasn't thinking of wrapping the AlignedCharArrayUnion in a union, I was thinking of using it *as* the union.

Here's one way you could do it. In the first commit, you can change:

union {
  struct NT NamedType;
  struct CXXOpName CXXOperatorName;
  struct CXXLitOpName CXXLiteralOperatorName;
};

to something like:

AlignedCharArrayUnion<NT, CXXOpName, CXXLitOpName> NameLoc;

This will mean updating users of NamedType, CXXOperatorName, and CXXLiteralOperatorName to go through the new NameLoc field. If there are a lot of them, you might want to start with a prep commit that adds:

NT &getNamedType() { return NamedType; }
CXXOpName &getCXXOperatorName() { return CXXOperatorName; }
CXXLitOpName &getCXXLiteralOperatorName() { return CXXLiteralOperatorName; }

and change all the users to go through the function, then when you land the change to AlignedCharArrayUnion you just have to update those three functions. But if there aren't many users it may not be worth it.

Once you've created NameLoc (using an AlignedCharArrayUnion), a follow-up commit can change CXXOpName and CXXLitOpName to use a SourceLocation instead of a PODSourceLocation, and then you can delete PODSourceLocation once you've handled all of its users.