Added missing 'const' qualifier to many functions' parameters #324
No reviewers
Labels
No labels
bug
duplicate
enhancement
feature
help wanted
improve API
invalid
needs documentation
not-planned
rtfm
stale
support
wishlist
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-libtom/libtomcrypt!324
Loading…
Reference in a new issue
No description provided.
Delete branch "fix-missing-const-params"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I have found several functions taking non-cost pointers, while the only thing they do is return size of some keys.
I believe that functions like these really should take their arguments as
const rsa_key*to preserve const-correctness and not being forced to write (i. e. in C++) things likeinline int GetKeySize() const { return ::rsa_get_size(const_cast<rsa_key*>(&key_)); }If someone comes across another function(s) which should have
constparameter but do not, you can write here and we can fix them all at once instead of splitting that into several commits.Thanks!
EDIT: As was pointed out by several commenters, there are many more functions in which
a non-const pointer parameter is not used to change the pointed-to object, therefore the pointer can be declared as a const pointer. See below for the full discussion.
There's a huge bunch of functions that could use a
const*, probably @fperrad can create a list from its linter?I guess this could be the start of tackling the API improvements :)
I agree that we should handle this consistently at least over all
rsa_*,ecc_*,dh_*,dsa_*Here a first list (151 items).
Note: this kind of work is incremental, when the first founds are fixed, the linter could find the next ones.
damn 😆
@TrinityCoder go go go xD
@rofl0r OK, I will start with the list provided by @fperrad and will look forward to see what comes next :)
I just realized that we have to be careful when changing this in the 3 different implementations of
ltc_math_descriptor... I'd say you can probably leave them out or the code will most likely become splattered by casts back to non-const because ltm doesn't have a release withconst*arguments yet and tfm doesn't even have it implemted...Thanks @sjaeckel for the comment and sorry for taking long to reply.
CURRENT STATUS
I have taken a look at all those errors coming from the Travis build, and the main problem is probably in errors similar to this one (which actually counts for the majority of error log):
These errors arise because now we are correctly passing
constpointers inside our code,but we have not changed anything in i. e.
libtommathitself, which also uses the "incorrect"non-const pointer approach...
SOLUTION
Let me think a few minutes about this...
tommath.h:299:5: note: expected ‘struct mp_int *’ but argument is of type ‘const void *’this doesn't look like its only due to addition of
const@sjaeckel Sorry for requesting a review a few minutes ago; it was a missclick
@rofl0r Yes you are right, this error appears as well, I know about it and I will fix it
Cool, thanks for continuing work on this!
As already mentioned here, I think we can leave the math descriptors as they are for now.
No problem :) I removed myself from the reviewers list, can you please add me again as soon as you think it should be reviewed!
@sjaeckel Yes, I will try to fix all the functions (except the math descriptor ones...) and immediately tag you for review :)
@fperrad could you please merge this locally into develop and update your list w/o
src/math/*_desc.c?@TrinityCoder when you have the time to continue:
doc/crypt.texthx
Here the second list (79 items)
Please avoid changes to src/pk/ecc/* It would be better to handle these
after merging pr/ecc-* branches
👍
@fperrad can you please check again :)
Here the third list (32 items)
regarding
src/pk/asn1/der/sequence/der_decode_sequence_multi.c 132 Info 818: Pointer parameter 'a1' (line 29) could be declared as pointing to constI'm not sure if that's a good idea, as the c standard defines that theva_listarguments of the corresponding functions aren't const...and as already mentioned, the ecc stuff will be fixed after the ecc re-work
and @fperrad can you check again please? :)
Here the fourth list (16+2 items)
@fperrad and hopefully the last iteration :)
@sjaeckel could we merge this PR into develop?
after @fperrad checked a last time, yes
good for me (
src/pk/ecccurrently excluded).