search

BlackEdder / painlessjson

D library for converting any custom types to and from JSON the painless way.
Boost Software License 1.0
24 stars 3 forks source link

Compiler bug or painlessjson/traits bug #49

Closed BlackEdder closed 8 years ago

BlackEdder commented 8 years ago

@msoucy and @Zalastax I tried to compile painlessjson with the newest version, but get the following error (see travis failure for imports branch):

painlessjson ~imports: building configuration "__test__unittest__"...
../../../.dub/packages/painlesstraits-0.0.3/source/painlesstraits.d(36,9): Warning: statement is not reachable
../../../.dub/packages/painlesstraits-0.0.3/source/painlesstraits.d(36,9): Warning: statement is not reachable
../../../.dub/packages/painlesstraits-0.0.3/source/painlesstraits.d(36,9): Warning: statement is not reachable
../../../.dub/packages/painlesstraits-0.0.3/source/painlesstraits.d(36,9): Warning: statement is not reachable
../../../.dub/packages/painlesstraits-0.0.3/source/painlesstraits.d(36,9): Warning: statement is not reachable
source/painlessjson/painlessjson.d(603,13): Warning: statement is not reachable
source/painlessjson/painlessjson.d(603,13): Warning: statement is not reachable
dmd failed with exit code 1.

The statement looks reachable to me, what do you guys think? Compiler bug?

Zalastax commented 8 years ago

I think the warnings might be sane. The compiler should be able to see that the return true in the foreach is unconditional if for every overload if (__traits(compiles, getInstanceFromCustomConstructor!(T, overload, false)(JSONValue()))) evaluates to true. It then replaces the foreach with return true, blocking return false from being reachable. A solution not using multiple returns would be favourable so I'll look into if it's possible to use reduce instead.

Zalastax commented 8 years ago

Combining the fix in painlesstraits with https://github.com/BlackEdder/painlessjson/commit/9f9b94b3a47ada4ffdea16ef5dc5476c74cb9a06 makes everything pass for me on dmd 2.069. Map and reduce didn't work but this is quite elegant as well.

BlackEdder commented 8 years ago

But if non of the if statements are correct then it should reach the return false statement or am I missing something?

On Mon, 18 Jan 2016 23:07 Pierre Krafft notifications@github.com wrote:

Combining the fix in painlesstraits with 9f9b94b https://github.com/BlackEdder/painlessjson/commit/9f9b94b3a47ada4ffdea16ef5dc5476c74cb9a06 makes everything pass for me on dmd 2.069. Map and reduce didn't work but this is quite elegant as well.

— Reply to this email directly or view it on GitHub https://github.com/BlackEdder/painlessjson/issues/49#issuecomment-172677446 .

Zalastax commented 8 years ago

Yes if there are no valid constructors the function reaches the return false. But this is a compile time function so the foreach gets unrolled to just some if true return true; if false return true;, and thereby blocks the return false. Using an accumulator solves this by having just one return statement.

BlackEdder commented 8 years ago

The unrolling makes sense. Thanks for the explanation. Will wait for @msoucy to merge and increment painlesstraits' version number, and then merge imports.

Zalastax commented 8 years ago

The branch is ready for version bump and merge!

BlackEdder commented 8 years ago

Thanks @Zalastax and @msoucy