Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compile without warnings #347

Merged
merged 12 commits into from
Oct 9, 2019
Merged

Compile without warnings #347

merged 12 commits into from
Oct 9, 2019

Conversation

henrygab
Copy link
Collaborator

@henrygab henrygab commented Oct 1, 2019

This PR allows v0.14.0 to compile cleanly (no warnings), allowing end-users to focus on issues within their own code.

Note: I am not sure how to fully test these. I ran the bleuart example, and verified UART capability.

Of particular note is the changes to the InternalFileSystem library's flash_cache.c, function flash_cache_read(). Significant comments were added to help understand the implicit guarantees provided by the various code paths. A PowerShell function mirroring the original code flow was written to generate results from various parameters. A second PowerShell function mirroring the new code flow was written to validate those parameters ended up with the same actions occurring in the final code, including on all edge cases. Manual code review validated how the old code avoided some additional edge cases, and comments now explicitly document the implicit assumptions that the old code relied heavily upon for proper operation.

@henrygab
Copy link
Collaborator Author

henrygab commented Oct 5, 2019

Here's the simple PowerShell script I wrote to validate the logic changes:
https://gist.github.com/henrygab/d6d34d585ca3172f8ddf937658089af3

The first function references the line numbers as they exist now.
The second function corresponds to the logic in the PR.

Running the script will output the actions that would be taken for various inputs.
This includes where the parameters correspond to reads that:

  • are entirely before the cache area
  • end exactly at the start of the cache area
  • begin prior to, and partially overlap, the cache area
  • begin prior to, and extend past, the cache area
  • begin at start of cache area, and read only cache area
  • begin at start of cache area, and extend past the cache area
  • begin at offset into cache area, and read only cache area
  • begin at offset into cache area, and extends past the cache area
  • begin exactly at the end of the cache area
  • begin past the end of the cache area

Anyways, just wanted to show that work was done to validate the equivalence of the larger code change. Really, it's just a slight organizational change to allow the use of unsigned variables.

Add verbose comments to show that the values changed from signed to
unsigned, for the current code, would always be positive values.

Add verbose comments to clarify the meaning of local variables.

Add verbose comments to show all three possible states at all key
computation points, for the higher cyclomatic complexity code block.
a function that uses the fields (two lines later).
@henrygab
Copy link
Collaborator Author

henrygab commented Oct 5, 2019

rebase'd to latest master (8dbe7d9)

@henrygab
Copy link
Collaborator Author

henrygab commented Oct 7, 2019

Updating to remove additional compiler warnings at debug level 2.

Copy link
Member

@hathach hathach left a comment

Choose a reason for hiding this comment

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

Thank you very much, fixing lots of terrible warnings. And putting lots of effort into explaining how the read cache work. I actually intended to write a little more for it, but then hmmm, I guess I forgot :D. That is indeed very useful write-up :) . Great works!!!

@hathach
Copy link
Member

hathach commented Oct 9, 2019

Here's the simple PowerShell script I wrote to validate the logic changes:
https://gist.github.com/henrygab/d6d34d585ca3172f8ddf937658089af3

The first function references the line numbers as they exist now.
The second function corresponds to the logic in the PR.

Running the script will output the actions that would be taken for various inputs.
This includes where the parameters correspond to reads that:

  • are entirely before the cache area
  • end exactly at the start of the cache area
  • begin prior to, and partially overlap, the cache area
  • begin prior to, and extend past, the cache area
  • begin at start of cache area, and read only cache area
  • begin at start of cache area, and extend past the cache area
  • begin at offset into cache area, and read only cache area
  • begin at offset into cache area, and extends past the cache area
  • begin exactly at the end of the cache area
  • begin past the end of the cache area

Anyways, just wanted to show that work was done to validate the equivalence of the larger code change. Really, it's just a slight organizational change to allow the use of unsigned variables.

Thank you very much, this is very thoughtful of yours. Lots of effort to verify this, I should have done a better job of documenting :). Brilliant works and superb PR :)

@hathach hathach merged commit b9d586b into adafruit:master Oct 9, 2019
@henrygab henrygab deleted the PatchMulti branch October 9, 2019 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants