Skip to content

doc: refactor hashes group#9858

Merged
PeterKietzmann merged 14 commits intoRIOT-OS:masterfrom
jia200x:pr/doc_hashes
Aug 30, 2018
Merged

doc: refactor hashes group#9858
PeterKietzmann merged 14 commits intoRIOT-OS:masterfrom
jia200x:pr/doc_hashes

Conversation

@jia200x
Copy link
Copy Markdown
Member

@jia200x jia200x commented Aug 29, 2018

Contribution description

Following #9842, this PR groups all hashes into the hashes group. I also define/modify some other groups in order to create the categories Checksums, Keyed cryptographic hash functions, Unkeyed cryptographic hash functions and Non cryptographic hash functions (all of them under sys_hashes group).

This PR does the following:

  • Checksums moved from sysgroup to sys_hashes group
  • Define groups for sha256 and sha3
  • Add CMAC to sys_hashes_keyed
  • Add MD5, SHA1, SHA256 and SHA3 to sys_hashes_unkeyed
  • Add all hashes from hashes.h files to sys_hashes_non_crypto

I also deprecated the section based documentation from the doc.txt, because it was outdated and it had the same information as the Doxygen group list.

Testing procedure

make doc. Go to System>Hashes in the generated documentation and see how these components are grouped.

Issues/PRs references

#8479

@jia200x jia200x requested review from aabadie and smlng August 29, 2018 15:17
@jia200x jia200x added Area: doc Area: Documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 29, 2018
@jia200x jia200x mentioned this pull request Aug 29, 2018
Open
20 tasks
PeterKietzmann
PeterKietzmann reviewed Aug 30, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@PeterKietzmann PeterKietzmann left a comment

Choose a reason for hiding this comment

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

Nice! Did you also have a look at the header files in sys/include/checksum/?

sys/checksum/doc.txt
/**
* @defgroup sys_checksum Checksum
* @ingroup sys
* @ingroup sys_hashes
Copy link
Copy Markdown
Member

@PeterKietzmann PeterKietzmann Aug 30, 2018

Choose a reason for hiding this comment

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

Why do you put checksum in sys_hashes? I would guess that (as long as we keep both checksum/ and hashes/ in separate folders) we would keep both @ingroup sys ?

Copy link
Copy Markdown
Member Author

@jia200x jia200x Aug 30, 2018

Choose a reason for hiding this comment

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

As I have read, strictly speaking checksums are hashes. See for instance this entry.
Although they are in different folder, I'm trying to organize them by topic :)

Copy link
Copy Markdown
Member

@PeterKietzmann PeterKietzmann Aug 30, 2018

Choose a reason for hiding this comment

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

I was referring to the mismatch between folder structure and documentation. But I agree that it looks way nicer this way. So the conclusion might be to consider file shifting which is not the purpose of this PR and maybe not wanted anyway.

Copy link
Copy Markdown
Member Author

@jia200x jia200x Aug 30, 2018

Choose a reason for hiding this comment

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

Ah, I see. Yes, could be an option. I would be in for moving the folder in another PR

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 30, 2018

Nice! Did you also have a look at the header files in sys/include/checksum/?

Yes, since I move the sys_checksum group to sys_hashes, these headers are not affected by the change (the @ingroup directive is in charge of moving the group)

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Aug 30, 2018

these headers are not affected by the change

Sure, I was more thinking about the need for further cleanup. Otherwise I'm fine with this PR.

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 30, 2018

Sure, I was more thinking about the need for further cleanup. Otherwise I'm fine with this PR.

Good! :)

@PeterKietzmann PeterKietzmann merged commit 5b87b1d into RIOT-OS:master Aug 30, 2018
@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Aug 30, 2018

PS: I'm wondering if we need that many commits for this kind of change

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 30, 2018

PS: I'm wondering if we need that many commits for this kind of change

Maybe it doesn't make that much sense. I will try to commit by group rather than by feature.

@jia200x jia200x added this to the Release 2018.10 milestone Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PeterKietzmann PeterKietzmann PeterKietzmann approved these changes

@aabadie aabadie Awaiting requested review from aabadie

@smlng smlng Awaiting requested review from smlng

Assignees

No one assigned

Labels

Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2018.10

Development

Successfully merging this pull request may close these issues.

2 participants

@jia200x @PeterKietzmann