#1344 cppcheck report

Future Release
closed
None
9
2015-12-10
2012-05-18
serval24
No

Hello,

I just svn checkout Rosegarden project and launched cppcheck (git updated today).
I thought it could interest you, here are some examples :
[src/gui/dialogs/IntervalDialog.cpp:239] -> [src/gui/dialogs/IntervalDialog.cpp:223]: (style) Found duplicate if expressions
220 case 6: // seventh
221 if (deviation == -1)
222 textIntervalDeviated += tr("a minor");
223 else if (deviation == 0)
224 textIntervalDeviated += tr("a major");
225 else if (deviation == -2)
226 textIntervalDeviated += tr("a diminished");
227 else if (deviation == 1)
228 textIntervalDeviated += tr("an augmented");
229 else if (deviation == -3)
230 textIntervalDeviated += tr("a doubly diminished");
231 else if (deviation == 2)
232 textIntervalDeviated += tr("a doubly augmented");
233 else if (deviation == -4)
234 textIntervalDeviated += tr("a triply diminished");
235 else if (deviation == 3)
236 textIntervalDeviated += tr("a triply augmented");
237 else if (deviation == 4)
238 textIntervalDeviated += tr("a quadruply augmented");
239 else if (deviation == 0)
240 textIntervalDeviated += tr("a perfect");
241 else
242 textIntervalDeviated += tr("an (unknown, %1)").arg(deviation);
243 break;

[src/gui/dialogs/ImportDeviceDialog.cpp:115]: (performance) Possible inefficient checking for 'm_devices' emptiness.
115 if (m_devices.size() == 0) {

[src/gui/editors/eventlist/EventView.cpp:380]: (performance) Prefer prefix ++/-- operators for non-primitive types.
379 for (Segment::iterator it = m_segments[i]->begin();
380 m_segments[i]->isBeforeEndMarker(it); it++) {

I attach the complete report. Even it certainly contains false reports, I just
thought it could help. If you prefer some patches, tell me.

Julien.

Related

Bugs: #1342

Discussion

  • serval24

    serval24 - 2012-05-18

    complete cppcheck report on trunk

     
  • Ted Felix

    Ted Felix - 2012-05-19

    Patches would be great. If you can focus first on the problems that are easy to check by inspection (e.g. Prefer prefix ++/-- operators) that would make it easier for us to evaluate and accept the patches. For the harder issues (e.g. the duplicate if expressions example) if you can provide a test case that would exercise them along with the patch, that would be very helpful. Sometimes it is hard to tell how to exercise a piece of code as rosegarden is very large.

    And thanks for turning me on to cppcheck. Looks like a handy tool.

     
  • serval24

    serval24 - 2012-05-25

    patch to fix "Prefer prefix..." reported by cppcheck

     
  • serval24

    serval24 - 2012-05-25

    I attached the patch which is quite straightforward. Sorry for the delay.

    Julien

     
  • Ted Felix

    Ted Felix - 2012-05-25

    I reviewed this patch. It looks good. Thanks for this. Did you do all this work by hand, or did you automate it in some way?

     
  • serval24

    serval24 - 2012-05-25

    Ok for the patch. About it, I tried to use the most automatic way, searched about mass editing.
    Badfully, I don't know enough about regexp so I generated a file to reduce things to do.
    I retrieved a file which contained things like :
    <file> +
    With one console on this file and with another one, I copy pasted on with "vi" (I'm on Linux) on prefix (+ allows to open the file directly on the good line)
    I think it must have taken about 1 hour once the file generated.

     
  • Ted Felix

    Ted Felix - 2012-05-25

    That was still a lot of work to do. Thanks. I've committed your first patch in r12943. Looks like there may have been a few postfix increment/decrements that were missed. You might want to re-run cppcheck and see if it finds any more. Maybe they were basic types and cppcheck ignored them.

     
  • serval24

    serval24 - 2012-06-02

    Patch to fix " Possible inefficient checking for 'xxx' emptiness

     
  • serval24

    serval24 - 2012-06-02

    I attached the patch about emptiness.

    About rerunning cppcheck, I'll do it later. But it's very easy, just git clone, make, then use it this way :
    cd <rosegarden_install_directory>trunk/rosegarden
    <cppcheck_directory>/cppcheck --enable=all ./ 2>reports_cppcheck.txt

    Julien

     
  • Ted Felix

    Ted Felix - 2012-06-02

    Thanks. At first glance, empty.patch looks good. I'm going to comb through the changes a little more carefully and do a little testing as I apply this. Should have it done in a day or two.

     
  • D. Michael McIntyre

    Status?

     
  • Tom Breton

    Tom Breton - 2012-12-07

    This is very helpful. However, it's not a single bug that can be open or closed. Right now some of those are fixed and others aren't. There's no status like "30% closed" that can reflect that.

     
  • serval24

    serval24 - 2012-12-07

    In fact, this tracker was just to show the interest of cppcheck (a free soft).
    I don't know if a "task" more than "bug" exists or perhaps "meta bug"?

    I use git version, it's very easy to compile and run.
    Here's the git repo:
    https://github.com/danmar/cppcheck.git

    If you want, I can retrieve last sources of Rosegarden and post an updated full report.

    Another interesting thing would be to try to compile with Clang

     
  • D. Michael McIntyre

    • milestone: 12.12 --> Future Release
     
  • Ted Felix

    Ted Felix - 2013-05-02

    These look pretty legit:

    [src/base/Composition.cpp:1776]: (error) Dangerous iterator usage after erase()-method.
    [src/document/io/LilyPondExporter.cpp:358]: (error) Dangerous iterator usage after erase()-method.
    [src/document/io/LilyPondExporter.cpp:411]: (error) Dangerous iterator usage after erase()-method.
    [src/sound/Tuning.cpp:379]: (error) Dangerous iterator usage after erase()-method.
    

    Recommend my "increment before use" idiom as a relatively simple fix. Need to test thoroughly, though.

    // Roughly, replace the for's with:
    iterator i = begin();
    while (i != end())
    {
        // Increment before use
        iterator j = i++;
    
        if (j.isEvil())
        {
            // Safe now.
            erase(j);
        }
    }
    
     
  • Ted Felix

    Ted Felix - 2013-09-25

    Eclipse/Java regex to find leading underscores on names:

    \b_[_a-zA-Z]+
    

    1,645 matches right now. Mostly #include guards. I think a tool is in order for fixing all of these. Perhaps an awk script.

     
  • D. Michael McIntyre

    Why does that matter again? I can't remember, and didn't find the answer digging around on this bug report. I'm guessing it breaks name mangling on 128-bit systems running CP/M using the special proprietary ARM compiler or something impressively obscure like that.

    I mean we've had those include protector thingies for 13 years.

    Not that I'm telling you not to cobble up a bit of awk to change them into something else. Knock yourself out! I'm just curious why this seemingly silly undertaking is necessary.

     
  • Ted Felix

    Ted Felix - 2013-09-25

    Names starting with underscores are reserved for the compiler vendors. Of course, these are preprocessor names, so we probably aren't running afoul of that rule. However, the thousands of include guards breaking the rules do make it a bit harder to find the handful of real rule-breakers. That's the theory I'm going with. I've got a sed script and a patch ready to go. Going to float it by the dev list....

     
  • Ted Felix

    Ted Felix - 2014-03-27

    Ok, I've re-run cppcheck and fixed those that I could test easily. There appear to be two legitimate ones that remain. I did get a warning from cppcheck that it didn't check all #ifdef configurations, so more may be lurking.

    The two that appear legitimate looked rather difficult to test, so I left those alone. There were also a number of false positives.

    Appear Legitimate

    [src/sound/Tuning.cpp:373] -> [src/sound/Tuning.cpp:379]: (error) Iterator 'it' used after element has been erased.

    • Tuning::Tuning()
    • This one looks like a valid problem.

    [src/document/io/HydrogenXMLHandler.h:126]: (style) Class 'HydrogenXMLHandler' is unsafe, 'HydrogenXMLHandler::m_segment' can leak by wrong usage.

    • Could be an issue. In HydrogenXMLHandler::endElement_093(), if the segment has no events in it, it is not freed. See the "sequence" tag handler.

    False Positives

    [src/document/io/LilyPondExporter.cpp:329] -> [src/document/io/LilyPondExporter.cpp:360]: (error) Iterator 'k' used after element has been erased.

    • LilyPondExporter::handleEndingPreEvents()
    • This is a false positive. The code is too complex for cppcheck to follow. It could be simplified like this:

      eventendlist::iterator k = preEventsInProgress.begin();
      
      while (k != preEventsInProgress.end()) {
          // Increment before use.  This avoids invalidating k if the element
          // at l is erased.
          eventendlist::iterator l(k++);
      
          // Use "l" instead of "k".
          // ...
      
          // DO NOT do this at the end of the loop:
          // k = l;
      }
      

    [src/document/io/LilyPondExporter.cpp:381] -> [src/document/io/LilyPondExporter.cpp:413]: (error) Iterator 'k' used after element has been erased.

    • LilyPondExporter::handleEndingPostEvents()
    • Same false positive as previous. Same solution recommended.

    [src/commands/matrix/MatrixInsertionCommand.h:53]: (style) Class 'MatrixInsertionCommand' is unsafe, 'MatrixInsertionCommand::m_lastInsertedEvent' can leak by wrong usage.

    • False positive. cppcheck doesn't realize that the segment takes ownership of the event.

    [src/commands/matrix/MatrixPercussionInsertionCommand.h:60]: (style) Class 'MatrixPercussionInsertionCommand' is unsafe, 'MatrixPercussionInsertionCommand::m_lastInsertedEvent' can leak by wrong usage.

    • False positive. cppcheck doesn't realize that the segment takes ownership of the event.

    I'm going to take my name off this bug report as I believe this is all I can do. If there's no point in pursuing the remaining items, this can be closed.

     
  • Ted Felix

    Ted Felix - 2014-03-27
    • assigned_to: Ted Felix --> nobody
     
  • Ted Felix

    Ted Felix - 2015-12-10
    • status: open --> closed
    • assigned_to: Ted Felix
     
  • Ted Felix

    Ted Felix - 2015-12-10

    I've fixed the ones that were easy to test. Comments have been added to point out the one in Tuning.cpp.

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks