Parse C++ warnings more specifically #1852

jougs · 2020-11-16T15:50:34Z

No description provided.

heplesser · 2020-11-16T21:24:02Z

@jougs 👍! I'd be very happy to review when its past the draft stage :).

heplesser

@jougs Well done, much nicer than my #1842. Just one little suggestion for efficiency ;).

extras/parse_travis_log.py

jougs · 2020-11-16T21:45:59Z

Looking at the few "real" warnings now, I think we can even eliminate them completely. I'll add a printout of all found warnings to this PR, but would file another one to get them fixed.

heplesser · 2020-11-16T22:15:34Z

Looking at the few "real" warnings now, I think we can even eliminate them completely. I'll add a printout of all found warnings to this PR, but would file another one to get them fixed.

Most of them are in a very deep part of the SLI scanner, where some case-fallthroughs may be intentional. If you dare to touch such ancient code ;).

The remainder seems to come from the way be block out MPI code if building without MPI, that should be easily fixable by moving all HAVE_MPIs out of functions.

…cordingly

jougs · 2020-11-16T23:22:34Z

@heplesser: actually, I don't dare ;-)

In 1f2f5c4 I've now refactored the MPIManager and MUSICManager along the lines you suggested and think we should just live with the remaining four warnings in SLI. I'll revisit the two conngen related warnings in the FULL build when merging these changes into the branch #1830 is based off.

heplesser

@jougs Thanks, just two small glitches (one confirmed by Travis, one suspected).

nestkernel/mpi_manager.cpp

nestkernel/music_manager.cpp

heplesser · 2020-11-17T06:28:33Z

@heplesser: actually, I don't dare ;-)

Maybe @diesmann would dare descend into the depths of the SLI scanner?

Co-authored-by: Hans Ekkehard Plesser <[email protected]>

jougs · 2020-11-17T08:32:40Z

@heplesser: actually, I don't dare ;-)

Maybe @diesmann would dare descend into the depths of the SLI scanner?

We could just try adding breaks to the statements and see if anything breaks 😉 If not, we're probably safe - the times where people get adventurous with new SLI code are likely to be over and the code used from within the SLI library and PyNEST should be mostly stable.

hakonsbm · 2020-11-17T08:49:52Z

@jougs I tried adding breaks there in in my previous PR (8a2eefa). It broke the SLI scanner, so I figured it was best to just live with it (914e420).

jougs · 2020-11-17T08:56:50Z

Thanks @hakonsbm. Another possibility would be to ignore those four manually and just not count them.

heplesser

👍 Looks good now and all tests have passed (macOS still running and expected to fail due to #1839). I suggest to merge this once @hakonsbm has also approved, then @lekshmideepu can merge into #1839 and all will be well again :).

heplesser · 2020-11-17T10:10:27Z

Thanks @hakonsbm. Another possibility would be to ignore those four manually and just not count them.

I would leave them in, we handle them nicely now. The scanner code is actually quite interesting, but to figure out precisely how to replace the fall-throughs would require a deeper understanding of the state machine than I have time to acquire right now (or blatant copy-pasting of the code one calls through to). But definitely not in this PR ;).

jougs · 2020-11-17T10:17:25Z

@heplesser

I would leave them in, we handle them nicely now.

Argh! I went ahead and silenced them in 4c64c3c just two minutes before I saw your comment. Should I revert?

... would require a deeper understanding of the state machine than I have time to acquire right now

And even if you had time, I bet you'd find a a better use for it somewhere else ;-)

diesmann · 2020-11-17T10:18:01Z

Hi, the break and the absence of it has a a clearly defined function. As far as I remeber the case statements in the scanner code have been carefullt designed to respect this. Changing the code likely changes its meaning. Regards, Markus

…

On 11/17/20 11:10 AM, Hans Ekkehard Plesser wrote: Thanks @hakonsbm <https://github.com/hakonsbm>. Another possibility would be to ignore those four manually and just not count them. I would leave them in, we handle them nicely now. The scanner code is actually quite interesting, but to figure out precisely how to replace the fall-throughs would require a deeper understanding of the state machine than I have time to acquire right now (or blatant copy-pasting of the code one calls through to). But definitely not in this PR ;). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1852 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADEB5FKBR7J6HDSWJEZCTE3SQJD2FANCNFSM4TXKVJBQ>.

-- Prof. Dr. Markus Diesmann, Director Inst of Neuroscience and Medicine (INM-6) Tel +49-2461-61-4748 (secr) Computational and Systems Neuroscience & +49-2461-61-9301 (lab) Institute for Advanced Simulation (IAS-6) Fax +49-2461-61-9460 Theoretical Neuroscience Mob +49-175-290-1943 Jülich Research Centre and JARA Institute I Jülich, Germany Department of Psychiatry, Psychotherapy and Psychosomatics School of Medicine, RWTH Aachen University Aachen, Germany www.nest-initiative.org www.csn.fz-juelich.de

heplesser · 2020-11-17T10:19:41Z

Argh! I went ahead and silenced them in 4c64c3c just two minutes before I saw your comment. Should I revert?

No keep them in :)!

hakonsbm · 2020-11-17T11:50:05Z

@jougs @heplesser Isn't it better to revert and add them to the expected warnings?

heplesser · 2020-11-17T12:01:00Z

@jougs @heplesser Isn't it better to revert and add them to the expected warnings?

Almost a philosophical question. By exempting some, but not all warnings that are known to us, have been evaluated as not problematic and difficult to fix, we split warnings into two categories, the expliclitly ignored and the ignored-by-count. Is this split warranted, or should we treat all on an equal footing? By just considering a count, it could happen that some warnings disappear and are just replaced by other warnings, while explicit checks avoid this. One problem with the explicit warnings is that they will resurface if a change in a different part of a source file changes line numbers.

Given that this blocks our CI pipeline, I am much in favor of a decent working solution now and delegation of philosophical matters to a follow-up issue.

hakonsbm · 2020-11-17T12:22:33Z

I suppose with only a handful warnings it is sensible to ignore them explicitly as known warnings. However, the check for known warnings doesn't work right now so the known warnings are not ignored and Travis fails. Since it's matching the whole line, the known warning strings may be missing newlines at the end.

jougs · 2020-11-17T13:11:45Z

@hakonsbm: I've added a call to strip() in ae7da9f, hoping that this resolves the problem.

hakonsbm

Looks like using strip() fixes the problem, so I'm approving.

jougs · 2020-11-18T04:42:27Z

OK. My most recent commit now also fixed the final four warnings on the Clang-build and we're down to 0 (in numbers: zero!) in all but the libneurosim-enabled build (but the two currently expected warnings there might go away with a merge of #1830).

The Mac OS X build is failing expectedly due to #1839.

jougs · 2020-11-18T04:43:34Z

Given the two positive reviews and the fact that I'm up early, I'm just merging myself.

jougs added 2 commits November 16, 2020 14:28

Parse warnings more specifically

f8670e0

Adapt expected number of warnings

3d1893e

jougs requested review from hakonsbm and heplesser November 16, 2020 21:25

jougs self-assigned this Nov 16, 2020

jougs added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Critical Needs to be addressed immediately T: Maintenance Work to keep up the quality of the code and documentation. labels Nov 16, 2020

jougs added this to the NEST 3.0 milestone Nov 16, 2020

heplesser requested changes Nov 16, 2020

View reviewed changes

extras/parse_travis_log.py Outdated Show resolved Hide resolved

This was referenced Nov 16, 2020

Update NumPy installation on Mac OS X Travis build #1839

Merged

Softer checks for compiler warnings in CI #1842

Closed

jougs added 2 commits November 17, 2020 00:15

Compile regular expression to increase performance of warning scan

54893be

Reduce number of warnings and adapt expected numbers in log parser ac…

1f2f5c4

…cordingly

jougs marked this pull request as ready for review November 16, 2020 23:23

heplesser requested changes Nov 17, 2020

View reviewed changes

nestkernel/mpi_manager.cpp Outdated Show resolved Hide resolved

nestkernel/music_manager.cpp Outdated Show resolved Hide resolved

Apply suggestions from code review

ee5efc6

Co-authored-by: Hans Ekkehard Plesser <[email protected]>

heplesser approved these changes Nov 17, 2020

View reviewed changes

jougs added 2 commits November 17, 2020 11:08

Ignore warnings from SLI and make regular expresssion more generic

4c64c3c

Simplify

1833897

jougs added 2 commits November 17, 2020 11:12

Make variable name less screamy

fcef630

Merge branch 'travisci' of github.com:jougs/nest-simulator into travisci

20dd576

Compare stripped line to ignore trailing newline

ae7da9f

hakonsbm approved these changes Nov 17, 2020

View reviewed changes

Include implementation header instead of regular header

e322613

jougs merged commit 1ac75d4 into nest:master Nov 18, 2020

jougs mentioned this pull request Nov 18, 2020

Fix Wundefined-var-template warnings #1851

Closed

heplesser mentioned this pull request Nov 19, 2020

Remove errors caused by explicit .colorize.rst include #1795

Closed

hakonsbm mentioned this pull request Apr 14, 2021

Parse build output of GitHub Actions #2012

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse C++ warnings more specifically #1852

Parse C++ warnings more specifically #1852

jougs commented Nov 16, 2020

heplesser commented Nov 16, 2020

heplesser left a comment

jougs commented Nov 16, 2020

heplesser commented Nov 16, 2020

jougs commented Nov 16, 2020

heplesser left a comment

heplesser commented Nov 17, 2020

jougs commented Nov 17, 2020

hakonsbm commented Nov 17, 2020

jougs commented Nov 17, 2020

heplesser left a comment

heplesser commented Nov 17, 2020

jougs commented Nov 17, 2020

diesmann commented Nov 17, 2020 via email

heplesser commented Nov 17, 2020

hakonsbm commented Nov 17, 2020

heplesser commented Nov 17, 2020

hakonsbm commented Nov 17, 2020

jougs commented Nov 17, 2020

hakonsbm left a comment

jougs commented Nov 18, 2020

jougs commented Nov 18, 2020

Parse C++ warnings more specifically #1852

Parse C++ warnings more specifically #1852

Conversation

jougs commented Nov 16, 2020

heplesser commented Nov 16, 2020

heplesser left a comment

Choose a reason for hiding this comment

jougs commented Nov 16, 2020

heplesser commented Nov 16, 2020

jougs commented Nov 16, 2020

heplesser left a comment

Choose a reason for hiding this comment

heplesser commented Nov 17, 2020

jougs commented Nov 17, 2020

hakonsbm commented Nov 17, 2020

jougs commented Nov 17, 2020

heplesser left a comment

Choose a reason for hiding this comment

heplesser commented Nov 17, 2020

jougs commented Nov 17, 2020

diesmann commented Nov 17, 2020 via email

heplesser commented Nov 17, 2020

hakonsbm commented Nov 17, 2020

heplesser commented Nov 17, 2020

hakonsbm commented Nov 17, 2020

jougs commented Nov 17, 2020

hakonsbm left a comment

Choose a reason for hiding this comment

jougs commented Nov 18, 2020

jougs commented Nov 18, 2020