[Gardening] Use string constants for Builtin module and type names #14963
Conversation
|
I think we should fix |
kitasuke
changed the title
|
@swift-ci please smoke test |
kitasuke
changed the title
Delete redundant comment Use upper case for constant
|
@harlanhaskins I also added |
lib/AST/ASTPrinter.cpp
Outdated
| @@ -3228,21 +3228,23 @@ class TypePrinter : public TypeVisitor<TypePrinter> { | |||
| llvm::raw_svector_ostream UnderlyingOS(UnderlyingStrVec); | |||
| T->getElementType().print(UnderlyingOS); | |||
| } | |||
| if (UnderlyingStrVec.startswith("Builtin.")) | |||
| std::string builtin_name = BUILTIN_NAME; | |||
| std::string builtin_type_prefix = builtin_name + "."; | |||
There was a problem hiding this comment.
Since we dynamically construct this a few times, we should maybe just make a new constant for “Builtin.”
There was a problem hiding this comment.
Good point. Fixed it.
|
@harlanhaskins Addressed your feedback! Would be appreciated if you could review again! |
include/swift/Strings.h
Outdated
| /// The name of the Builtin type for Int512 | ||
| static const char BUILTIN_TYPE_NAME_INT512[] = "Builtin.Int512"; | ||
| /// The name of the Builtin type for Float | ||
| static const char BUILTIN_TYPE_NAME_Float[] = "Builtin.Float"; |
There was a problem hiding this comment.
Should this be _FLOAT vs _Float?
There was a problem hiding this comment.
Sorry for trivial mistake... Fixed it.
kitasuke
force-pushed
the
replace-of-builtin-string-with-constant
branch
from
5690487 to
d433428
Compare
|
I squashed and pushed the commit, thanks for reviewing! |
|
LGTM! @swift-ci please smoke test and merge |
|
Thanks @kitasuke! |
I defined new string constants in
String.hfor Builtin so that we could replace string value with it.BUILTIN_NAMEBUILTIN_TYPE_NAMEsIt might be a bit different case, but should I also apply to Builtin type name like here?https://github.com/apple/swift/blob/master/lib/IDE/TypeReconstruction.cpp#L795:L797Here is a link for the discussion.
#14959 (comment)