This is an archive of the discontinued LLVM Phabricator instance.

Make getObjCEncodingForTypeImpl() take a bitmask instead of 8 bools
ClosedPublic

Authored by thakis on May 10 2019, 8:16 AM.

Diff Detail

Event Timeline

thakis created this revision.May 10 2019, 8:16 AM
rjmccall added inline comments.May 10 2019, 10:44 AM
clang/include/clang/AST/ASTContext.h
2877

I like the idea of doing this, but can you add some boilerplate? :) I think it'd be better if this were a struct with some nice accessors, factories, transformations, and so on.

This example isn't from Clang, but something like this (without the templating, of course): https://github.com/apple/swift/blob/14a20eea03e9115e2c5cf91bccc86e6cd5334df9/include/swift/ABI/MetadataValues.h#L118

thakis updated this revision to Diff 199056.May 10 2019, 12:33 PM
thakis edited the summary of this revision. (Show Details)

Now with more boilerplate!

thakis marked 2 inline comments as done.May 10 2019, 12:34 PM
thakis added inline comments.
clang/include/clang/AST/ASTContext.h
2877

Done. It got pretty wordy (+30 lines instead of -30 before), so I x-macro'd it a bit.

rjmccall added inline comments.May 10 2019, 12:49 PM
clang/include/clang/AST/ASTContext.h
2877

That looks good, but please camelCase the method names. Also, the method names sound like they mutate this rather than returning a value with the mutation in effect.

clang/lib/AST/ASTContext.cpp
6885

These two uses can probably be hoisted up as a common operation, basically "do this for a component of the original type". forComponentType(), maybe?

thakis updated this revision to Diff 199116.May 10 2019, 6:38 PM
thakis marked 4 inline comments as done.

comments

clang/include/clang/AST/ASTContext.h
2877

Made them mutate this, except for keepOnly() which I renamed keepingOnly() and marked LLVM_NODISCARD, and clear which is now forComponentType() and also LLVM_NODISCARD.

Made all methods except the accessors camelCase. (Making the first letter lower case as-is is tricky with the xmacro, and isExpandPointedToStructures sounds strange. hasExpandPointedToStructures sounds better, but hasIsStructField is weird.)

thakis updated this revision to Diff 199408.May 14 2019, 5:28 AM

minor tweaks for landing

This revision was not accepted when it landed; it landed in state Needs Review.May 14 2019, 5:30 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2019, 5:30 AM