Summary:	ASTERISK-07940: [patch] Improve const-correctness
Reporter:	Markus Elfring (elfring)	Labels:
Date Opened:	2006-10-17 04:31:41	Date Closed:	2011-06-07 14:08:27
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
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.. http://svn.digium.com/view/asterisk/team/anthonyl/8160-branch/ 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 * NOT 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. http://groups.google.de/group/comp.lang.c/msg/06262f96b1511636 http://groups.google.de/group/comp.lang.c/tree/browse_frm/thread/f9a239c13f1e646e/7ddeb64f049f72e1 http://groups.google.de/group/comp.lang.c/tree/browse_frm/thread/4ed93378bb31394a/4bc03cba54dcb355 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. http://c-faq.com/decl/strlitinit.html 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 FAILED FAILED FAILED 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 1.4.1.7992". 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... http://cygwin.com/ml/cygwin/2007-01/msg00290.html 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.