Address Dead Code in the Slice Compilers #3188
Conversation
| @@ -562,7 +562,6 @@ optional | |||
| { | |||
| auto scoped = dynamic_pointer_cast<StringTok>($2); | |||
| ContainerPtr cont = currentUnit->currentContainer(); | |||
| assert(cont); | |||
There was a problem hiding this comment.
currentContainer can never return null:
ContainerPtr
Slice::Unit::currentContainer() const
{
assert(!_containerStack.empty());
return _containerStack.top();
}
| @@ -405,19 +405,6 @@ Slice::Type::usesClasses() const | |||
| // Builtin | |||
| // ---------------------------------------------------------------------- | |||
|
|
|||
There was a problem hiding this comment.
typeId is only used in error messages in two places.
Both places didn't care about this Ice:: prefix either.
I inlined the logic in the 2 places where it was needed.
| @@ -1238,7 +1227,6 @@ Slice::Container::createClassDecl(const string& name) | |||
| } | |||
| } | |||
|
|
|||
| _unit->currentContainer(); | |||
There was a problem hiding this comment.
currentContainer is const. All it does is return a reference to the current container.
So, calling this function and ignoring the return value is pointless.
| @@ -1870,22 +1857,6 @@ Slice::Container::modules() const | |||
| return result; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Unused code.
| @@ -2146,11 +2102,6 @@ Slice::Container::validateConstant( | |||
| { | |||
| // isConstant indicates whether a constant or a data member (with a default value) is being defined. | |||
|
|
|||
There was a problem hiding this comment.
It's impossible for type to be nullptr here.
In the grammar file at the top of this PR, the type rule used to return nullptr if there's no currentContainer.
But there is always a container, so this nullptr return was impossible, meaning it's impossible to hit here.
| @@ -3790,23 +3700,6 @@ Slice::Exception::usesClasses() const | |||
| return false; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
More dead code.
| @@ -250,9 +250,9 @@ Slice::filterMcppWarnings(const string& message) | |||
| { | |||
| static const char* messages[] = {"Converted [CR+LF] to [LF]", "no newline, supplemented newline", 0}; | |||
|
|
|||
There was a problem hiding this comment.
No reason not to use constexpr string_view instead...
| @@ -666,10 +666,22 @@ Gen::TypesVisitor::visitEnum(const EnumPtr& p) | |||
| void | |||
| Slice::Gen::TypesVisitor::visitConst(const ConstPtr& p) | |||
| { | |||
There was a problem hiding this comment.
The place where I inlined the implementation for the typeId method.
| @@ -183,99 +183,7 @@ Slice::CsGenerator::fixId(const string& name, unsigned int baseTypes, bool mangl | |||
| string | |||
| Slice::CsGenerator::getOptionalFormat(const TypePtr& type) | |||
| { | |||
There was a problem hiding this comment.
The code coverage report marked Slice::Type::getOptionalFormat as unused.
But instead of deleting it, the correct fix was to start using it.
Every single language has it's own getOptionalFormat which is just a re-implementation of the original one, with some prefix slapped on front. So I deleted all the redundant code, and now they all just call the getOptionalFormat defined in the parser...
| @@ -372,7 +372,7 @@ namespace | |||
| { | |||
| BuiltinPtr b = dynamic_pointer_cast<Builtin>(p); | |||
| assert(b); | |||
There was a problem hiding this comment.
The 2nd place where we used to typeId. But here we already know that we're dealing with a builtin type, so using the virtual function typeId was pointless anyways. I switched it to just use the kindAsString method which returns int, string, Value, etc. and is specific to built-in types.
@externl's new coverage report was a useful navigation tool that led me to a few chunks of unused code.
Most of this code is just deleted by this PR, but there were things that should of been being used by the compilers instead.
I'm just opening this PR to let CI double check that I haven't broken anything.