Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#1344 cppcheck report

Future Release
open
nobody
None
9
2014-03-27
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

     
    Attachments
  • 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

     
    Attachments
  • 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

     
    Attachments
  • 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.

     
  • 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

     
    • 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.

     
  • 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