Wrap return value in std::move (Clang 7 build fix) #3301
Conversation
karkhaz
changed the title
karkhaz
force-pushed
the
kk-clang7-build-fix
branch
from
276edb4 to
645dce0
Compare
karkhaz
requested review from
cesaro,
cristina-david,
NathanJPhillips,
peterschrammel,
pkesseli,
smowton and
thk123
as code owners
| @@ -1251,14 +1251,14 @@ exprt java_string_library_preprocesst::get_primitive_value_of_object( | |||
| typecast_exprt(object, pointer_type), pointer_type.subtype()); | |||
| member_exprt deref_value(deref, value_comp.get_name(), value_comp.type()); | |||
| code.add(code_assignt(value, deref_value), loc); | |||
| return value; | |||
| return std::move(value); | |||
There was a problem hiding this comment.
What a bore. I thought RVO meant you didn't need to move locals on return...
There was a problem hiding this comment.
Yes, I am also bugged by this. The rule isn't hard ('the type must match'), but it's still super annoying.
There was a problem hiding this comment.
Fair - so in this case could be fixed by changing return to symbol_exprt? This seems like the better fix since the docs explicitly says returns a symbol
There was a problem hiding this comment.
Except ... it doesn't - there is a branch that returns a nil_exprt.
There was a problem hiding this comment.
And presumably if you did return optionalt<symbol_exprt>(...) then that would need to be std::move'd as well?
There was a problem hiding this comment.
I don't actually think so, it would likely be the best approach. Well, even if the std::move were required, it would still be cleaner.
There was a problem hiding this comment.
I'll try to use optionalt since I have a compiler to test with, I'll let you know how it goes in a few moments...
There was a problem hiding this comment.
OK, please have a look at the updated patch. Indeed, std::move is not required. get_primitive_value_of_object now returns an optionalt<symbol_exprt>, and I've updated its only caller accordingly...please let me know if the call site looks wrong, I'm not sure that I did the right thing.
|
@thk123 it's not "required to compile," it's actually a warning that can be disabled with |
karkhaz
force-pushed
the
kk-clang7-build-fix
branch
2 times, most recently
from
2f504f5 to
678b730
Compare
| if(value.has_value()) | ||
| code_not_null.add(code_assignt(field_expr, *value), loc); | ||
| else | ||
| code_not_null.add(code_assignt(field_expr, nil_exprt()), loc); |
There was a problem hiding this comment.
Puh, I'm pretty sure this is broken and has actually been all along? @romainbrenguier maybe?
There was a problem hiding this comment.
The else case should never happen, it would be better to put an invariant here.
The nil_exprt case happen for name == ID_void, which is excluded by the if condition.
This fixes diffblue#3238 (again).
karkhaz
force-pushed
the
kk-clang7-build-fix
branch
from
678b730 to
d6e1984
Compare
There was a problem hiding this comment.
🚫
This PR failed Diffblue compatibility checks (cbmc commit: 645dce0).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90853799
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.
|
Do I need to rebase, or will the compatibility checks pick up my latest push automatically @allredj? I resolved all of @tautschnig's suggestions except one. |
|
Heh yeah I consider warnings as not compiling since we compile with warnings as errors! |
|
@karkhaz I think you should leave it - Joels bot refers to an old commit id so I imagine the new one is still running. |
There was a problem hiding this comment.
🚫
This PR failed Diffblue compatibility checks (cbmc commit: 678b730).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90863299
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.
There was a problem hiding this comment.
✔️
Passed Diffblue compatibility checks (cbmc commit: d6e1984).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90866577
This fixes #3238 (again).