Page MenuHomePhabricator

[clang][ASTImporter] Add import of thread safety attributes.
ClosedPublic

Authored by balazske on Sep 27 2021, 3:15 AM.

Details

Summary

Attributes of "C/C++ Thread safety attributes" section in Attr.td
are added to ASTImporter. The not added attributes from this section
do not need special import handling.

Diff Detail

Event Timeline

balazske created this revision.Sep 27 2021, 3:15 AM
balazske requested review of this revision.Sep 27 2021, 3:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2021, 3:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I appreciate the effort in working on the attribute imports.

Personally, I don't really like this change. IMO AttrImporter::createImportedAttr() should do the magic, and apply the necessary glue for the import.
Or we could even create a dedicated function, which would 'just do the right thing' and import the Attr, without the current noise of calling the individual imports than gluing the stuff to the constructor dispatch.

The proposed implementation involves too much code repetition for me.

The code can be made better in a way that only lines similar to

Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
    From, AI.importArg(From->getSuccessValue()).value(),
    AI.importArrayArg(From->args(), From->args_size()).value(),
    From->args_size());

are needed. If there is an AttrVisitor the switch can be removed. No one import function is possible that works with every Attr, in the same way as there is a separate function for import of every Stmt (Decl and others are more complicated) there must be something for every Attr (if it has custom arguments). Even now the import of an Attr needs much less code than import of a Stmt (that could be done in similar way). There are cases that need special handling, like AlignedAttr. If the current code is included we can see enough cases to make a better generalization, but after splitting the ASTImporter.cpp into multiple files.

martong accepted this revision.Sep 28 2021, 2:04 AM

I agree with @steakhal , that perhaps we could come up with a somewhat more compact and elegant implementation in the future. Nevertheless, I think this patch in its current form is already valuable and a nice work, it does what it needs to do.

This revision is now accepted and ready to land.Sep 28 2021, 2:04 AM

If the ASTImporter and parts of ASTNodeImporter are made public API then it can be possible to add generation of import code to the .td files, this should be the best solution. In similar way, import of Stmt and Expr nodes can be generated if possible. These imports are mechanical tasks that can be automated, probably with some exceptions.

If the ASTImporter and parts of ASTNodeImporter are made public API then it can be possible to add generation of import code to the .td files, this should be the best solution. In similar way, import of Stmt and Expr nodes can be generated if possible. These imports are mechanical tasks that can be automated, probably with some exceptions.

Okay, let's keep that to a follow-up patch.

This revision was landed with ongoing or failed builds.Oct 5 2021, 3:54 AM
This revision was automatically updated to reflect the committed changes.