31 May
2016
31 May
'16
1:19 p.m.
Review at https://gerrit.osmocom.org/136
RFC: remove this == NULL checks
The compile should ensure that this == NULL should never happen
Change-Id: Ifddaef70bb0a4402050c817b1000d515c3a7118b
---
M src/llc.h
M src/tbf.cpp
2 files changed, 2 insertions(+), 5 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/36/136/1
diff --git a/src/llc.h b/src/llc.h
index 94de16e..4883624 100644
--- a/src/llc.h
+++ b/src/llc.h
@@ -127,10 +127,10 @@
inline size_t gprs_llc_queue::size() const
{
- return this ? m_queue_size : 0;
+ return m_queue_size;
}
inline size_t gprs_llc_queue::octets() const
{
- return this ? m_queue_octets : 0;
+ return m_queue_octets;
}
diff --git a/src/tbf.cpp b/src/tbf.cpp
index 69b9e3a..51705e2 100644
--- a/src/tbf.cpp
+++ b/src/tbf.cpp
@@ -1181,9 +1181,6 @@
const char *gprs_rlcmac_tbf::name() const
{
- if (this == NULL)
- return "(no TBF)";
-
snprintf(m_name_buf, sizeof(m_name_buf) - 1,
"TBF(TFI=%d TLLI=0x%08x DIR=%s STATE=%s%s)",
m_tfi, tlli(),
--
To view, visit https://gerrit.osmocom.org/136
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifddaef70bb0a4402050c817b1000d515c3a7118b
Gerrit-PatchSet: 1
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
31 May
31 May
1:46 p.m.
New subject: osmo-pcu[master]: RFC: remove this == NULL checks
Patch Set 1: Code-Review+1
I would agree that a non-static method cannot ever be caleld with this == NULL, but
I'm not a C++ expert...
--
To view, visit https://gerrit.osmocom.org/136
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddaef70bb0a4402050c817b1000d515c3a7118b
Gerrit-PatchSet: 1
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Harald Welte <laforge(a)gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-HasComments: No
1 Jun
1 Jun
12:30 p.m.
New subject: osmo-pcu[master]: RFC: remove this == NULL checks
Patch Set 1: Code-Review-1
Here's my guess:
If a class has no virtual functions, the functions are essentially static and merely get
passed the 'this' pointer of the instance called upon.
For an inline foo() const function, the compiler is instructed to insert the code
in-place, nevermind it being a class member namespace wise.
The actual value of 'this' happens during runtime. The compiler can try hard to
avoid NULL there, but it's easy to achieve at runtime.
MyClass *instance = NULL;
instance->my_non_virtual_function();
In this code, the NULL 'this' is actually passed into the function body.
(For a virtual function, the program would first try to look up the function pointer in
the virtual function table, which would probably result in a segfault.)
Plus: Jacob wrote it. My opinion of Jacob's compiler knowledge is highly esteemed.
Let's ask him first. Plus: Coverity warned about one of those.
On the counter argument, the compiler does complain about comparison of 'this'
against NULL. However, I trust that Jacob had his reasons in these particular instances.
Would be nice to get rid of the warning though.
Giving -1 but it's more like a -0 until we have more precise knowledge.
--
To view, visit https://gerrit.osmocom.org/136
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddaef70bb0a4402050c817b1000d515c3a7118b
Gerrit-PatchSet: 1
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Harald Welte <laforge(a)gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr(a)sysmocom.de>
Gerrit-HasComments: No
1:14 p.m.
New subject: osmo-pcu[master]: RFC: remove this == NULL checks
Patch Set 1:
all checks of (this == null) will be eliminated by GCC >= 6.1
(https://gcc.gnu.org/gcc-6/changes.html, Value range propagation now assumes that the this
pointer of C++ member functions is non-null. This eliminates common null pointer checks
but also breaks some non-conforming code-bases (such as Qt-5, Chromium, KDevelop). As a
temporary work-around -fno-delete-null-pointer-checks can be used. Wrong code can be
identified by using -fsanitize=undefined.)
But given the "make check" failure we seem to expect that. You will need to move
the null check before calling the function or create a method wrapper where the tbf
pointer is passed so we can do the null check there.
--
To view, visit https://gerrit.osmocom.org/136
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddaef70bb0a4402050c817b1000d515c3a7118b
Gerrit-PatchSet: 1
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Harald Welte <laforge(a)gnumonks.org>
Gerrit-Reviewer: Holger Freyther <holger(a)freyther.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr(a)sysmocom.de>
Gerrit-HasComments: No
4:53 p.m.
New subject: osmo-pcu[master]: RFC: remove this == NULL checks
Patch Set 1:
all checks of (this == null) will be eliminated by GCC
>= 6.1
(https://gcc.gnu.org/gcc-6/changes.html, Value range propagation
now assumes that the this pointer of C++ member functions is
non-null. This eliminates common null pointer checks but also
breaks some non-conforming code-bases (such as Qt-5, Chromium,
KDevelop). As a temporary work-around -fno-delete-null-pointer-checks
can be used. Wrong code can be identified by using
-fsanitize=undefined.)
But given the "make check" failure we seem to expect that. You will
need to move the null check before calling the function or create a
method wrapper where the tbf pointer is passed so we can do the
null check there.
thanks for the explanation.
this is also the reason, why make check fails on my laptop.
gcc --version
gcc (GCC) 6.1.1 20160501
--
To view, visit https://gerrit.osmocom.org/136
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddaef70bb0a4402050c817b1000d515c3a7118b
Gerrit-PatchSet: 1
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Harald Welte <laforge(a)gnumonks.org>
Gerrit-Reviewer: Holger Freyther <holger(a)freyther.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-HasComments: No
2892
days inactive
2893
days old
4 comments
4 participants
participants (4)
-
Harald Welte -
Holger Freyther
-
lynxis lazus
-
Neels Hofmeyr