[Safety] Remove C Style Casts #6967
Conversation
|
I had a hard time finding an implementation of this in the changes, so for anyone else looking: public:
template <class TARGET>
TARGET &Cast() {
D_ASSERT(dynamic_cast<TARGET *>(this));
return (TARGET &)*this;
}
template <class TARGET>
const TARGET &Cast() const {
D_ASSERT(dynamic_cast<const TARGET *>(this));
return (const TARGET &)*this;
}I wonder if this can't be done in a generic fashion, instead of requiring to be implemented on the type itself Cast<FunctionExpression>(expr);Using some template magic, we should be able to replicate the const/non-const separation I think it should also be able to check for the existence of a TYPE variable with enable_if, and overload on that TARGET &Cast() {
if (expression_class != TARGET::TYPE) {
throw InternalException("Failed to cast expression to type - expression type mismatch");
}
return (TARGET &)*this;
}I think Max did something similar recently for the serializer/deserializer But we might prefer to have this as an explicit function on the type, so we can easily alter it to the type and have the error be as descriptive as possible, instead of wading through layers of templating to make a meaningful change |
|
I thought about trying that but opted not to for various reasons - most |
|
Nice! In any case if type is deducible ahead of time, it will be already properly code-generated (with the checking elided and either the call or the throw known), so that is a big nice gain. |
|
The thread sanitizer error is caused by the change to the concurrentqueue.hpp header guard, probably need to add to
|
| @@ -382,7 +382,7 @@ unique_ptr<BenchmarkState> InterpretedBenchmark::Initialize(BenchmarkConfigurati | |||
| state->con.Query("PRAGMA enable_profiling"); | |||
| state->con.Query("PRAGMA profiling_mode='detailed'"); | |||
| } | |||
| return state; | |||
| return std::move(state); | |||
There was a problem hiding this comment.
Looks like you ran into #6964, I can't seem to reproduce this.
There are quite a lot of places we rely on this behavior, no?
Or I guess the issue lies in not being able to implicitly convert a unique_ptr of a derived class to the base class, but when we move it, the move constructor kicks in and that does allow the conversion
There was a problem hiding this comment.
No, GCC-4.9 does not support it for the regular std::unique_ptr so we already had std::move all over the place for that compiler. I ran into it on a Linux box while compiling the benchmark module
This PR removes most usages of C-style casts when casting to derived classes, such as when casting from e.g. a
ParsedExpressionto aFunctionExpression, or aLogicalOperatorto aLogicalGet, etc. Instead, we replace these casts with a templatedCastfunction that returns a reference to the derived member. For example, this:Now becomes this:
This has a number of advantages over C-style casts:
Castfunction will check that the cast is to the correct type (e.g. for expressions by looking at theexpression_classvariable) - an InternalException is thrown when a cast to an incorrect type is performedCastfunction cannot remove const-ness of values by accident. Ifexpris aconst, then the returned value is alsoconst.Castfunction will only compile if called correctly, as opposed to C-style casts. For example, this will compile (and crash):But this will result in a compilation error: