Fix #831, Resolve int size mismatch in loop comparison

[skliper]

Describe the contribution
Fix #831 - resolves loop iterator size too small for comparison

Testing performed
Build and unit test passed.

Expected behavior changes
Resolves LGTM warning

System(s) tested on

• Hardware: cFS Dev Server
• OS: Ubuntu 18.04
• Versions: bundle main (+ cfe/osal main) + this change.

Additional context
None

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
Jacob Hageman - NASA/GSFC

[CDKnightNASA]

(note that the CFE_EVS_SendEventWithAppID() calls without whitespace after commas between parameters makes the diff display hard to read:

1028                    CFE_SB_UnlockSharedData(...
1029
          CFE_EVS_SendEventWithAppID(...

Not something to fix for this ticket, and probably not something to fix generally. Just something to consider for formatting standards.)

[skliper]

I would like to apply our standard formatting style on cFE at some point... just hard to do in a way to minimize impacts on all the open PR's. I'd think this would help. Maybe do it in pieces (UT, then by service or something), and get it reviewed checked in right away.

[CDKnightNASA]

I would like to apply our standard formatting style on cFE at some point... just hard to do in a way to minimize impacts on all the open PR's. I'd think this would help. Maybe do it in pieces (UT, then by service or something), and get it reviewed checked in right away.

I'm wondering if there's a way to do this where we check binaries before vs. after mass formatting changes to be sure this didn't impact functionality? Certainly would be good to declare a point where this would be done so developers working on various tickets would know when to merge from main and address the impacts.

[skliper]

check binaries...

Good idea.

declare a point...

Another good idea. Needs to be done before the certification code reviews, which I haven't scheduled yet. There is probably no good time, just times that are less disruptive.

[astrogeco]

CCB 2020-08-26 - APPROVED

[CDKnightNASA]

Another good idea.

Here's another good idea...I'm de-facto the benevolent dictator for SBN, how about we do a trial with SBN and see how smoothly it goes there? I just finished up my stubs and coverage test code for SBN and the UDP module (almost 1.5 months since I started, yikes. And I'm sure it can use some cleanup and improvement!) So I'm up for breaking things again. Happy to help develop the "apply style" scripts.

[skliper]

I wrote myself an apply style script... just does find . -name "*.[ch]" -exec clang-format-10 -i -style=file {} +. I'd do the pre-post binary check by hand.

[CDKnightNASA]

I wrote myself an apply style script... just does find . -name "*.[ch]" -exec clang-format-10 -i -style=file {} +. I'd do the pre-post binary check by hand.

Would it also be worthwhile to have Travis run this and warn/err if there's a significant delta?

[skliper]

I put style enforcement in one repo (ci_lab I think), but have gotten quite a bit of push-back from @jphickey so I haven't enforced anywhere else. I agree w/ his points that it makes managing an active repo challenging, but I still think as disciplined/professional FSW developers we could handle it. At minimum we do need to apply it prior to certification code reviews.

[jphickey]

And I still maintain that the tool is very useful, but not perfect.

It does some weird/non-ideal formatting of certain constructs which make it LESS readable than it was before it was applied - statically defined lists/globals, long-ish argument lists to printf calls, etc. come to mind. Worse for C++ code but still happens in C too. It also has the tendency to "fix" things that weren't really broken or badly formatted to begin with.

I'm fine with the approach of applying the style mechanically using the tool at a certain pre-release development point when there are no major changes in work (not now!).

But ultlmately a human should have the authority to review the changes and accept/reject/adjust the whitespace changes that the tool does, because ultimately the whole point of the exercise is to make the source more human-readable and nothing else. So humans should decide what's better. (Classic case of trying to use an algorithm for a human job - it works well 98% of the time, but the other 2% becomes a very annoying nuisance).

[CDKnightNASA]

I wrote myself an apply style script... just does find . -name "*.[ch]" -exec clang-format-10 -i -style=file {} +. I'd do the pre-post binary check by hand.

FYI I ran this on SBN today and you can see the result in the SBN repo. Is there a way to turn off whitespace diff in github's display? I can't find a way. :/

I had to load SBN into Ubuntu 'cause Debian 10.5 has clang-format-7 which don't work. Ubuntu "focal" (20.04) has clang-format-10 which seemed to run without issue. Ubuntu "bionic" (18.04) seems to have clang-format-6, so running clang-format on Travis wouldn't work without going to "focal".

[skliper]

FYI I ran this on SBN today and you can see the result in the SBN repo. Is there a way to turn off whitespace diff in github's display? I can't find a way. :/

Yes, it's under the gear icon. Just a checkbox to ignore white space.

I had to load SBN into Ubuntu 'cause Debian 10.5 has clang-format-7 which don't work. Ubuntu "focal" (20.04) has clang-format-10 which seemed to run without issue. Ubuntu "bionic" (18.04) seems to have clang-format-6, so running clang-format on Travis wouldn't work without going to "focal".

See https://github.com/nasa/ci_lab/blob/6d9010a23ae9a996311ee322dcee4b2b0cd8cbfa/.travis.yml#L16-L21 for the way to install 10 on bionic.

[CDKnightNASA]

Aaah, I get the "gear icon" when looking at pull requests, but looking at commit deltas it doesn't show that icon...

nasa/SBN@e1a5c23

better example:

nasa/SBN@74b142b