Minor 'ice2slice' Fixes #3827
Minor 'ice2slice' Fixes #3827
Conversation
| const char* | ||
| Slice::FileException::ice_id() const noexcept | ||
| { | ||
| return "::Slice::FileException"; | ||
| } | ||
|
|
There was a problem hiding this comment.
This exception is only used internally by the Slice compilers.
We never check or care about it's id. So why bother defining it.
There was a problem hiding this comment.
Whenever we define a LocalException, it needs to be a proper LocalException, with an ice_id(). We should never "half-define" a LocalException.
There was a problem hiding this comment.
It is now a runtime_error instead of a LocalException.
It really has no business being a LocalException since it doesn't interact at all with the Ice runtime.
And none of the functionality offered by LocalExecption was ever used by it.
| // First check if any 'xxx:identifier' has been applied to this element. | ||
| // If so, we return that instead of the element's Slice identifier. | ||
| const string metadata = languageName + ":identifier"; | ||
| const string metadata = unit()->languageName() + ":identifier"; | ||
| if (auto customName = getMetadataArgs(metadata)) |
There was a problem hiding this comment.
This assert was to prevent us from calling mappedName in ice2slice (ice2slice should always use the Slice name).
But, there are helper functions in libSlice which use this function; Indirectly causing ice2slice to crash.
So I removed the assert.
We can self-police ourselves to not use mappedName in ice2slice.
| @@ -610,11 +611,11 @@ Gen::TypesVisitor::visitSequence(const SequencePtr& p) | |||
| } | |||
| out << "<" | |||
| // TODO the generic argument must be the IceRPC C# mapped type | |||
| << typeToString(p->type(), p->scope(), false) << ">\")]"; | |||
| << typeToString(p->type(), scope, false) << ">\")]"; | |||
There was a problem hiding this comment.
We already store p->scope() in a variable.
No need to keep calling it over and over.
| @@ -704,6 +704,7 @@ Gen::TypesVisitor::getOutput(const ContainedPtr& contained) | |||
| { | |||
| string outputName = getOutputName(_fileBase, _modules.size() > 1 ? scope : ""); | |||
| unique_ptr<Output> out = make_unique<Output>(outputName.c_str()); | |||
| FileTracker::instance()->addFile(outputName); | |||
There was a problem hiding this comment.
Whenever a Slice compiler creates a file/directory, it should let FileTracker know.
For ice2slice specifically, it calls FileTracker::cleanup() if something goes wrong:
ice/cpp/src/ice2slice/Main.cpp
Lines 215 to 218 in ca07ff1
FileTracker which files were generated.
| // First check if any 'xxx:identifier' has been applied to this element. | ||
| // If so, we return that instead of the element's Slice identifier. | ||
| const string metadata = languageName + ":identifier"; | ||
| const string metadata = unit()->languageName() + ":identifier"; |
There was a problem hiding this comment.
We should add metadata validation to ice2slice, otherwise in theory you could us the :identifier metadata to get unexpected behavior here.
There was a problem hiding this comment.
That's actually a very sneaky find. I didn't see it, but you're right, a user could use :identifier to have an effect here.
But yes, we need to add validation which would reject bogus metadata like this. See: #3257.
This PR:
ice2slicewhere it wasn't registering generated files with theFileTracker.assertwhich causedice2sliceto crash when it saw an operation.