Summary:ASTERISK-07940: [patch] Improve const-correctness
Reporter:Markus Elfring (elfring)Labels:
Date Opened:2006-10-17 04:31:41Date Closed:2011-06-07 14:08:27
Versions:Frequency of
Environment:Attachments:( 0) app_db.c.rej
( 1) bug_8160_const4_patch_results.txt
( 2) const3.patch
( 3) const4.patch
Description:Would you like to apply the advices from the article "http://en.wikipedia.org/wiki/Const_correctness" to your sources at more places?
I suggest to add the key word "const" to the type specifiers of parameters like the following.
- cmd, parse (function "filter")
- tmpl (function "chan_misdn_log")
- state,  exten, callerid, format, port, c (function "misdn_new")
- name (function "create_name")
- exten (function "ast_get_extension_...")
Comments:By: Russell Bryant (russell) 2006-10-17 11:16:35

Yes, these types of changes are welcome.  Please open a report for each patch that you have developed and would like to submit.  Thanks!

By: Markus Elfring (elfring) 2006-10-17 13:14:20

I would prefer to keep the issue open until such lines in the source files will be polished to enhance the readability of the interfaces.

I am glad about your acceptance on this topic.

By: Anthony LaMantia (alamantia) 2006-10-17 15:22:16

this is going to be quite a bit of work, if you want it to effect the large pile of code that is asterisk... i have made a testing branch in subversion..


please submit the modifications for this here, it's based of the trunk version of asterisk as this is something we can place in 1.4 at this stange in  it's release cycle.

By: Anthony LaMantia (alamantia) 2006-10-17 16:52:37

also, i don't really see the usefullness of using const in ast_get_extension*
as there are to fre operations preformed in those functions to really justifiy it's usage i could be wrong. what are your opinions also, you will need to tell us what source files these functions are in

By: Russell Bryant (russell) 2006-10-17 20:14:28

elfring, are you working on a patch for the adding "const" to the arguments you have specified?

By: Serge Vecher (serge-v) 2006-10-23 13:25:33

elfring: ping

By: Markus Elfring (elfring) 2006-10-23 15:32:37

I will try it - the checkout is running ...

Some parts of the source files are const-correct already. How much do you care for the rest on your own?

By: Russell Bryant (russell) 2006-10-23 19:55:40

I wouldn't try to go through the entire source tree to figure out where "const" should be added.  If you'd like to work on this, just work on smaller areas at a time.

By: Markus Elfring (elfring) 2006-10-26 15:43:44

Can the appended changes be integrated into the sources?

By: Russell Bryant (russell) 2006-10-26 16:42:46

Can you please file a disclaimer with Digium?  Then, we will be able to review your patch for inclusion.

By: Russell Bryant (russell) 2006-11-09 22:03:46.000-0600

I can't even look at your code until you file a disclaimer with Digium.  Feel free to reopen this issue once you have done so.  Thanks!

By: Anthony LaMantia (alamantia) 2006-11-28 10:32:45.000-0600

disclaimer on file. re-opening.

By: Tilghman Lesher (tilghman) 2006-11-28 11:30:01.000-0600

I'd have an issue with this patch, as it changes the constification of some items incorrectly.  This patch makes a return pointer unchangeable, instead of (correctly) making the contents of that pointer unchangeable.

By: Markus Elfring (elfring) 2006-11-29 06:02:08.000-0600

Would you like to be more specific about the places where more tweaks would be needed?

By: Tilghman Lesher (tilghman) 2006-11-30 01:46:51.000-0600

Okay, your changes are technically correct, but they differ with the coding style that we use:

const char *
char const*

Any change that flips the order of the const keyword to after the type should be removed.

By: Markus Elfring (elfring) 2006-11-30 08:15:18.000-0600

I have performed a search and replace in the patch file for your prefered key word ordering.

The reason for my style can be found at:
- http://codecraft.pool-room.com/Cpp/const-correctness-3.html
- parashift.com/c++-faq-lite/const-correctness.html#faq-18.9

By: Tilghman Lesher (tilghman) 2006-11-30 10:46:00.000-0600

You still have "int const" instead of "const int".  You need to fix all specifiers.

By: Markus Elfring (elfring) 2006-11-30 13:14:15.000-0600

Will the patch be integrated into your repository after the last 12 reorderings?

By: Tilghman Lesher (tilghman) 2006-11-30 13:20:58.000-0600

Actually, now that I look at it, why are you even constifying the integers?  Those are passed by value, not passed by reference, so there's very little reason to protect them.

By: Serge Vecher (serge-v) 2006-11-30 14:39:59.000-0600

elfring: also, please upload in uncompressed format for ease of review. Thanks!

By: Markus Elfring (elfring) 2006-11-30 15:52:24.000-0600

I suggest to be strict with the const specifier.


By: Markus Elfring (elfring) 2006-12-27 17:22:58.000-0600

Is the interest on constified data structures growing?

By: Tilghman Lesher (tilghman) 2006-12-27 17:46:15.000-0600

Your latest patch does not apply cleanly to trunk.

By: Markus Elfring (elfring) 2006-12-28 11:40:38.000-0600

Would you like to be more specific about the integration problems?

I would like to provide a couple of fixes (my package from last night) for the initialisation of character arrays with string literals.

Are the intended corrections acceptable?
Which clean-ups might have got unpleasant side effects at the moment?

By: Tilghman Lesher (tilghman) 2006-12-28 12:54:09.000-0600

cd asterisk-trunk
patch -p0 < ../path/to/patch

Is that explicit enough?

By: Serge Vecher (serge-v) 2006-12-28 14:02:27.000-0600

elfring: attached "bug 8160 const4 patch results.txt" shows specific failures when applied to a fresh checkout of svn trunk r. 49023

By: Markus Elfring (elfring) 2006-12-28 15:30:26.000-0600

serge-v: Thanks for the detailed error log. I am surprised that the file "const4.patch" can not be completely processed after it was generated by the program "TortoiseSVN".

Do see any more unexpected things in the *.rej files? ("saving rejects ...")

I hope that the file name "file_download.php" that you chose does not make a remarkable difference here.

By: Serge Vecher (serge-v) 2006-12-31 11:28:00.000-0600

I'm attaching a sample reject file. I think that the patch file just needs to be redone against the latest svn to accommodate the updates since the time you've uploaded it. Also I would suggest using standard procedures for making a patch, as outlined on http://www.asterisk.org/developers/Patch_Howto

By: Markus Elfring (elfring) 2006-12-31 14:48:38.000-0600

I have got the impression that the patch program has got hiccups.
The TortoiseSVN GUI does not show any version conflicts.

By: Tilghman Lesher (tilghman) 2006-12-31 17:50:25.000-0600

elfring:  please use the command line tools to generate your patch.  You will not be told again.  We don't care what your GUI shows; your patch has conflicts.

By: Russell Bryant (russell) 2007-01-18 11:54:57.000-0600

I would be happy to apply this patch once it is updated to apply cleanly to the trunk and compile ...

By: Markus Elfring (elfring) 2007-01-22 07:50:26.000-0600

Thanks for your patience.

I would like to know the real reasons for the rejections by the patch program...

By: Serge Vecher (serge-v) 2007-01-29 09:24:35.000-0600

I would like to know whether you are going to address the suggestions by the development team and provide an updated trunk patch.

By: Russell Bryant (russell) 2007-02-07 15:18:15.000-0600

I'm closing this out.  Feel free to re-open if you are able to generate a patch that can apply to svn trunk.