Correction of some bitwise operations in GCode.cpp

[esspe2]

Convert direct Bitwise operations by macros defined in NutsAndBolts.h, less error-prone

[MaslowCommunityGardenRobot]

Congratulations on the pull request @esspe2

Now we need to decide as a community if we want to integrate these changes. You should vote by giving this comment a thumbs up or a thumbs down. Votes are counted in 48 hours. Ties will not be merged.

I'm just a robot, but I love to see people contributing so I'm going vote thumbs up (but my vote won't count...)!

[BarbourSmith]

I am not in a position right now to test this on hardware so I am not going to vote, but this generally looks good to me 👍

[blurfl]

I think this will need some testing. It compiles, but... It looks to me like these functions from NutsAndBolts.h treat the second parameter as a mask instead of as a single bit number as the original code did. That clears, sets or test more bits that intended.

// These are nifty functions from Grbl
#define bit_true(x,mask) (x) |= (mask)
#define bit_false(x,mask) (x) &= ~(mask)
#define bit_istrue(x,mask) ((x & mask) != 0)
#define bit_isfalse(x,mask) ((x & mask) == 0)

The pre-defined Arduino functions bitSet() and bitClear() work on single bits as the original code does.

The logic test in the line

 if (line_flags & LINE_FLAG_COMMENT_PARENTHESES) { line_flags &= ~(LINE_FLAG_COMMENT_PARENTHESES); }

seems plain enough without changing it, though it might be re-written as

if (line_flags & LINE_FLAG_COMMENT_PARENTHESES) { bitClear(line_flags, LINE_FLAG_COMMENT_PARENTHESES); } 

to clarify the bit-clearing operation. (As aprospero pointed out, this won't work because LINE_FLAG_COMMENT_PARENTHESES is a mask instead of a bit number.)

[esspe2]

Ok, I understand: beware of the definition, bit number vs bitmask, good point! By chance this flag is already defined as a bitmask so the macro still works here:
GCode.h:

define LINE_FLAG_COMMENT_PARENTHESES bit(0)

As for the readability, both ways seem valid, more a question of taste. I thought better to use macros defined for those cases, which also permit to avoid confusions between "!" and "~".

Well, in the last case the 'if' could even be dropped, since the only action is to clear the bit if it is set ;-) What do you think?

GCode.cpp: bitClear(line_flags, LINE_FLAG_COMMENT_PARENTHESES);

[aprospero]

esspe2 is right, the nutsandbolts macros from grbl use masks, the builtin arduino functions use bit numbers. Since LINE_FLAG_COMMENT_PARENTHESES is in fact a mask (which masks only one bit) bitClear can't be used in this context but bit_false is ok.

I'm with esspe2, the 'if' should be dropped. The compiler would dop it anyway and the results is easier to read.

[blurfl]

You're both right, the changes work as expected. 👍

[MaslowCommunityGardenRobot]

Time is up and we're ready to merge this pull request. Great work!