Hi Markus, Thanks for the code cleanups in trunk. One comment: please be careful in removing nullptr checks in trunk. Although they look unnecessary, we really don't want to (re-)add bugs. In rev 13460, you removed a this == nullptr check. It's terrible code, I agree. The problem is that some other bad code may still rely on that check. I'm not saying that's the case for that particular piece of code, but I'd rather not remove such checks in trunk until after release.
Small tip for people bughunting, "this" in a class method can be nullptr. A call through nullptr to non-virtual method will happily pass. Because we changed code like f(SPFoo *g) { if (!g) return; to Foo::f() { if (!this) return; you may see a check on "this" in the code. If you are sure that all callers make sure the object is not nullptr, you can remove the check in experimental.
thanks, Johan
Hi Johan, this is the actual warning:
../../cppify/src/sp-item.cpp:1267:11: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion] if (!(this && (SP_IS_TEXT(this) || SP_IS_USE(this)))) { ^~~~ ~~
../../cppify/src/sp-lpe-item.cpp:215:10: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion] if (!this) { ~^~~~
I read this as "I will optimize this check away anyway." I'm not sure whether that's the case.
Regards, Markus
-----Ursprüngliche Nachricht----- Von: Johan Engelen [mailto:jbc.engelen@...2592...] Gesendet: Mittwoch, 23. Juli 2014 20:54 An: Markus Engel Cc: Inkscape-Devel Betreff: Code cleanup in trunk
Hi Markus, Thanks for the code cleanups in trunk. One comment: please be careful in removing nullptr checks in trunk. Although they look unnecessary, we really don't want to (re-)add bugs. In rev 13460, you removed a this == nullptr check. It's terrible code, I agree. The problem is that some other bad code may still rely on that check. I'm not saying that's the case for that particular piece of code, but I'd rather not remove such checks in trunk until after release.
Small tip for people bughunting, "this" in a class method can be nullptr. A call through nullptr to non-virtual method will happily pass. Because we changed code like f(SPFoo *g) { if (!g) return; to Foo::f() { if (!this) return; you may see a check on "this" in the code. If you are sure that all callers make sure the object is not nullptr, you can remove the check in experimental.
thanks, Johan
GCC does not optimize out the check in my experience (-O2). The check /should/ be removed. Just not so close to release.
regards, Johan
On 23-7-2014 22:21, Markus Engel wrote:
Hi Johan, this is the actual warning:
../../cppify/src/sp-item.cpp:1267:11: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion] if (!(this && (SP_IS_TEXT(this) || SP_IS_USE(this)))) { ^~~~ ~~
../../cppify/src/sp-lpe-item.cpp:215:10: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion] if (!this) { ~^~~~
I read this as "I will optimize this check away anyway." I'm not sure whether that's the case.
Regards, Markus
-----Ursprüngliche Nachricht----- Von: Johan Engelen [mailto:jbc.engelen@...2592...] Gesendet: Mittwoch, 23. Juli 2014 20:54 An: Markus Engel Cc: Inkscape-Devel Betreff: Code cleanup in trunk
Hi Markus, Thanks for the code cleanups in trunk. One comment: please be careful in removing nullptr checks in trunk. Although they look unnecessary, we really don't want to (re-)add bugs. In rev 13460, you removed a this == nullptr check. It's terrible code, I agree. The problem is that some other bad code may still rely on that check. I'm not saying that's the case for that particular piece of code, but I'd rather not remove such checks in trunk until after release.
Small tip for people bughunting, "this" in a class method can be nullptr. A call through nullptr to non-virtual method will happily pass. Because we changed code like f(SPFoo *g) { if (!g) return; to Foo::f() { if (!this) return; you may see a check on "this" in the code. If you are sure that all callers make sure the object is not nullptr, you can remove the check in experimental.
thanks, Johan
Okay, I reverted these two changes.
Regards, Markus
-----Ursprüngliche Nachricht----- Von: Johan Engelen [mailto:jbc.engelen@...2592...] Gesendet: Mittwoch, 23. Juli 2014 22:25 An: Markus Engel Cc: 'Inkscape-Devel' Betreff: Re: AW: Code cleanup in trunk
GCC does not optimize out the check in my experience (-O2). The check /should/ be removed. Just not so close to release.
regards, Johan
On 23-7-2014 22:21, Markus Engel wrote:
Hi Johan, this is the actual warning:
../../cppify/src/sp-item.cpp:1267:11: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion] if (!(this && (SP_IS_TEXT(this) || SP_IS_USE(this)))) { ^~~~ ~~
../../cppify/src/sp-lpe-item.cpp:215:10: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion] if (!this) { ~^~~~
I read this as "I will optimize this check away anyway." I'm not sure whether that's the case.
Regards, Markus
-----Ursprüngliche Nachricht----- Von: Johan Engelen [mailto:jbc.engelen@...2592...] Gesendet: Mittwoch, 23. Juli 2014 20:54 An: Markus Engel Cc: Inkscape-Devel Betreff: Code cleanup in trunk
Hi Markus, Thanks for the code cleanups in trunk. One comment: please be careful in removing nullptr checks in trunk. Although they look unnecessary, we really don't want to (re-)add bugs. In rev 13460, you removed a this == nullptr check. It's terrible code, I agree. The problem is that some other bad code may still rely on that
check.
I'm not saying that's the case for that particular piece of code, but I'd rather not remove such checks in trunk until after release.
Small tip for people bughunting, "this" in a class method can be nullptr. A call through nullptr to non-virtual method will happily pass. Because we changed code like f(SPFoo *g) { if (!g) return; to Foo::f() { if (!this) return; you may see a check on "this" in the code. If you are sure that all callers make sure the object is not nullptr, you can remove the check in experimental.
thanks, Johan
participants (2)
-
Johan Engelen -
Markus Engel