Resolve gcc9 issues with bslmt_testutil.t.cpp #262
Conversation
gcc9 has changed the way it computes __LINE__ number when a macro
invocation spans multiple lines, specifically in the case of:
BSLMT_TESTUIL_LOOPX_ASSERT(I, J, K, L, M, N, // gcc 9+
some predicate); // gcc 8-
The line reported by this macro expansion changes from reporting
the second line in gcc8 and earlier (plus all other Bloomberg
supported compilers) to reporting the first line in gcc9 and later.
This change appears intentional, and following recommended best
practices for the upcoming C standard that specifically tries to
address this situation.
The issue that arises is comparing error-reporting strings in the
tests for the ASSERT family of macros. The solution, as only the
test driver is impacted, is to rename variables so that no line has
to wrap to conform to the BDE coding rules.
The second minor issues addressed is warnings from a couple of
member functions defined in the test machinary, but never used.
I opted to remove them entirely, on the understanding that if we
ever need a more fully featured 'OutputRedirector', there is a
perfectly good one sitting in the 'bsls_outputredictor' component,
complete with test driver.
|
Should I review this @AlisdairM ? (And do all the other things?) |
|
I just spent hours diagnosing this problem. It causes ball_log.t.cpp:10898 to fail on my compiler. That statement splits a BALL_LOGVA macro over two lines after hundreds of tests putting it on a single line. @AlisdairM I now see bslmt_testutil.t.cpp is also broken for me. Is bdls_testutil.t.cpp similarly afflicted by this LINE madness? It's also failing with inscrutable but suspicious assert messages. Any idea on how many/which tests are afflicted? I spent an hour whittling down ball_log.t.cpp to isolate the issue, then ran a test script through both circle and gcc-9 as a sanity check (both agree on first-line LINE reporting), then tore my hair out for another hour or two checking everything else for a problem, before trying out clang and older gccs for their verdict on LINE. I'll change my compiler to report the last line, as the path of least resistance, even though the morally righteous choice is first line. This kind of thing really doesn't belong in tests. |
|
I see your bslmt_testutil.t.cpp and raise you ball_logthrottle.t: Also bslmt_readerwriterlockassert.t.cpp. |
| @@ -1530,239 +1496,239 @@ int main(int argc, char *argv[]) | |||
| "====================================================\n"; | |||
|
|
|||
| enum { | |||
| BUFFER_SIZE = 1024, // size of the buffer used to store | |||
| // captured output | |||
| BUFFER_SIZE = 1024, // size of the buffer used to store | |||
There was a problem hiding this comment.
Yeah, if k_REPEAT has k_, I think this should have k_. Or neither.
There was a problem hiding this comment.
Yikes! This file has many more cleanups than I remember, but the NUM_ITERATIONS -> k_REPEAT was the only essential renaming, as we had to reduce name length so that 2 or 3 assertions would fit on a single line.
As long as adding the 'k_' does not introduce new line wrapping problems (that affect correctness) then switching to 'k_BUFFER_SIZE' for consistency would make sense - agreed.
There was a problem hiding this comment.
That is the one I can see. I do not know how many more constants are in this file. Maybe it is easier to remove the k_ from the changed one. Unless someone makes a REPEAT macro in a system header...
There was a problem hiding this comment.
Looking at the full file the inconsistency is already present, so I withdraw my comment, we did not introduce it.
gcc9 has changed the way it computes LINE number when a macro
invocation spans multiple lines, specifically in the case of:
BSLMT_TESTUIL_LOOPX_ASSERT(I, J, K, L, M, N, // gcc 9+
some predicate); // gcc 8-
The line reported by this macro expansion changes from reporting
the second line in gcc8 and earlier (plus all other Bloomberg
supported compilers) to reporting the first line in gcc9 and later.
This change appears intentional, and following recommended best
practices for the upcoming C standard that specifically tries to
address this situation.
The issue that arises is comparing error-reporting strings in the
tests for the ASSERT family of macros. The solution, as only the
test driver is impacted, is to rename variables so that no line has
to wrap to conform to the BDE coding rules. Then a consistent
value can be tested independent of platform.
The second minor issues addressed is warnings from a couple of
member functions defined in the test machinary, but never used.
I opted to remove them entirely, on the understanding that if we
ever need a more fully featured 'OutputRedirector', there is a
perfectly good one sitting in the 'bsls_outputredictor' component,
complete with test driver.
Issue number of the reported bug or feature request: #
Describe your changes
A clear and concise description of the changes you have made.
Testing performed
Describe the testing you have performed to ensure that the bug has been addressed, or that the new feature works as planned.
Additional context
Add any other context about your contribution here.