Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 348191

Summary: Quick fix for catch by value should propose to replace by a const reference
Product: [Tools] CDT Reporter: Corentin Jabot <corentinjabot>
Component: cdt-codanAssignee: Marc-André Laperle <malaperle>
Status: RESOLVED FIXED QA Contact: Elena Laskavaia <elaskavaia.cdt>
Severity: enhancement    
Priority: P3 CC: cdtdoug, malaperle, yevshif
Version: 8.0   
Target Milestone: 8.0.1   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
added catch by const ref
none
quick fix + test
none
quick fix + test
malaperle: iplog+
quick fix + test, modified malaperle: iplog-

Description Corentin Jabot CLA 2011-06-03 06:40:22 EDT 
Build Identifier: 20110526-1053

When catching an exception by value instead of by reference, the quick fixed actually proposed by Codan, is to replace with a non-const reference.

But, in most cases an exception should be catch by const-reference.
So, it would be great to propose this fix instead, or even better, to have both choices.


Reproducible: Always

Steps to Reproduce:
1.Run code analysis on a snippet involving catching an exception by value
2.
3.
Comment 1 Tomasz Wesolowski CLA 2011-06-16 09:50:11 EDT 
Created attachment 198105 [details]
added catch by const ref

So I assume we want to keep both options as possibilities?
Comment 2 Elena Laskavaia CLA 2011-07-15 21:21:03 EDT 
can you provide a junit test please?
Comment 3 Marc-André Laperle CLA 2011-07-15 21:55:08 EDT 
I see that this appends 'const &' instead of pre-pending const and appending &. I think most people do const Exception &, not Exception const &, what do you think ? And it would be cool to have the const & quick fix listed first in the list of quick fixes but I'm not sure how easy it is to do that.
Comment 4 Tomasz Wesolowski CLA 2011-07-19 15:59:33 EDT 
Created attachment 199945 [details]
quick fix + test

Added test cases.

Also followed the suggestion to catch (const Exception & name).

Alena, does Codan already have a facility for prioritizing quick fixes? I agree that it sounds useful.
Comment 5 Tomasz Wesolowski CLA 2011-07-19 16:05:06 EDT 
Created attachment 199946 [details]
quick fix + test

(Fixed some comments)
Comment 6 Marc-André Laperle CLA 2011-07-21 02:16:32 EDT 
Created attachment 200053 [details]
quick fix + test, modified

Hi Tomasz. I modified a bit your patch:
- Changed CatchByReference and CatchByConstReference to use pretty much the same logic (it's just a static method...)
- Handle the case where there is no declName (bug 352263) and add related test cases
- Add test to suite
- Minor copyright updates

What do you think of this patch?
Comment 7 Tomasz Wesolowski CLA 2011-07-21 11:50:30 EDT 
I've diffed the patches (gits gets helpful :)) and reviewed your changes. I appreciate this version, thank you for the valuable input!
Comment 8 Marc-André Laperle CLA 2011-07-21 13:05:27 EDT 
Thanks! Fixed in cdt_8_0 and master > 20110721.
Comment 9 CDT Genie CLA 2011-07-21 13:19:26 EDT 
*** cdt git genie on behalf of 348191 ***

    Bug 348191 - Quick fix for catch by value should propose to replace by a
    const reference
    Bug 352263 - [qf] Catch by reference quick fix places & at a wrong place
    when no space after the type

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=db669ba9633e67facd61a36cc34361d8363e1e56
Comment 10 CDT Genie CLA 2011-07-21 13:19:28 EDT 
*** cdt git genie on behalf of 348191 ***

    Bug 348191 - Quick fix for catch by value should propose to replace by a
    const reference
    Bug 352263 - [qf] Catch by reference quick fix places & at a wrong place
    when no space after the type

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=f434c8c5564989c2c0a578e74bde0ff7b70678de