Skip to content

Commit

Permalink
OpenZFS 9036 - zfs: duplicate 'const' declaration specifier
Browse files Browse the repository at this point in the history 
Authored by: Toomas Soome <[email protected]>
Reviewed by: Yuri Pankov <[email protected]>
Reviewed by: Matthew Ahrens <[email protected]>
Approved by: Dan McDonald <[email protected]>
Ported-by: Brian Behlendorf <[email protected]>

OpenZFS-issue: https://illumos.org/issues/9036
OpenZFS-commit: openzfs/openzfs@f02c28e434
Closes #6900
  • Loading branch information
@tsoome @behlendorf
tsoome authored and behlendorf committed Apr 14, 2018
1 parent eecdd8e commit cbb8933
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions module/zfs/vdev_indirect_mapping.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,8 @@ vdev_indirect_mapping_size(vdev_indirect_mapping_t *vim)
static int
dva_mapping_overlap_compare(const void *v_key, const void *v_array_elem)
{
const uint64_t * const key = v_key;
const vdev_indirect_mapping_entry_phys_t * const array_elem =
const uint64_t *key = v_key;
const vdev_indirect_mapping_entry_phys_t *array_elem =
v_array_elem;
uint64_t src_offset = DVA_MAPPING_GET_SRC_OFFSET(array_elem);

Expand Down

5 comments on commit cbb8933

@megari
Copy link
Contributor

@megari megari commented on cbb8933 Apr 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks odd. Unlike in the original Illumos patch, the const specifiers removed here are by no means duplicate. The point of the specifiers here was that key and array_elem themselves are const, which may help the compiler in optimization and also makes it complain if the variables are assigned to.

@megari
Copy link
Contributor

@megari megari commented on cbb8933 Apr 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that the "fix" in OpenZFS was an incorrect one. Instead, the const specifiers should have been moved to the other side of *, which was probably the intent of the original code and what was done in ZoL.

@ryao
Copy link
Contributor

@ryao ryao commented on cbb8933 Apr 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@megari It looks to me like what @tsoome should have done here is change this to be a constant pointer to constant data to match what we did. It is incredibly rare that anyone does that, but it is nice to specify that the pointer is constant when it is constant.

https://www.geeksforgeeks.org/const-qualifier-in-c/

His change probably doesn't hurt anything though.

@megari
Copy link
Contributor

@megari megari commented on cbb8933 Apr 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryao Exactly. Thank you for confirming.

@tsoome
Copy link
Contributor Author

@tsoome tsoome commented on cbb8933 Apr 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we can not know what was or was not the intent unless people will speak up. From this code we have function arguments 'const void *' and thats it, so the most natural thing to do is to keep the const specifiers as they are listed in arguments. It also is obvious that this code is not written in the stone, so we can update it, but it would be helpful to have some reasoning - guessing about possible intent is certainly not enough.

On another note - please upstream your stuff:)

Please sign in to comment.