bloomberg / bde

Basic Development Environment - a set of foundational C++ libraries used at Bloomberg.
Apache License 2.0
1.68k stars 318 forks source link

Resolve gcc9 issues with bslmt_testutil.t.cpp #262

Open AlisdairM opened 4 years ago

AlisdairM commented 4 years ago

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.

cppguru commented 4 years ago

Should I review this @AlisdairM ? (And do all the other things?)

seanbaxter commented 4 years ago

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.

seanbaxter commented 4 years ago

I see your bslmt_testutil.t.cpp and raise you ball_logthrottle.t: https://gist.github.com/seanbaxter/6b5231f7724c1169241a9b0ad5cab5fa

Also bslmt_readerwriterlockassert.t.cpp.