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

bug in flags checking in map.h? #10

Closed
dpinol opened this issue Jul 10, 2013 · 1 comment
Closed

bug in flags checking in map.h? #10

dpinol opened this issue Jul 10, 2013 · 1 comment

Comments

@dpinol
Copy link

dpinol commented Jul 10, 2013

Hi,
I get this error running cppcheck in gmapping.

/home/dani/svn/stacks/pal_navigation_slam/pal_gmapping/build/gmapping_export/grid/map.h:193: style: Suspicious expression. Boolean result is used in bitwise operation. The operator '!'and the comparison operators have higher precedence than bitwise operators. It is recommended that the expression is clarified with parentheses. [cppcheck: clarifyCondition]
/home/dani/svn/stacks/pal_navigation_slam/pal_gmapping/build/gmapping_export/grid/map.h:206: style: Suspicious expression. Boolean result is used in bitwise operation. The operator '!'and the comparison operators have higher precedence than bitwise operators. It is recommended that the expression is clarified with parentheses. [cppcheck: clarifyCondition]

Effectively, running the code below produces this, which does not much sense, unless the flags are not bit flags, in which case imho it does not make much sense to use binary operator &

0: Outside. not inside. Allocated. 
1: Outside. inside. Allocated. 
2: Outside. inside. Allocated. 
3: Outside. inside. Allocated. 

If the checks are changed to " if (! (s&Outside))" it makes a bit more sense, but not for Outside values.

0: not Outside. not inside. not Allocated. 
1: not Outside. inside. not Allocated. 
2: not Outside. not inside. Allocated. 
3: not Outside. inside. Allocated.

thanks

#include <iostream>

int main(int argc, char *argv[])
{
  enum AccessibilityState{Outside=0x0, Inside=0x1, Allocated=0x2};

  int state[] =  { Outside, Inside, Allocated, Inside|Allocated};
  for (int i=0; i < sizeof(state) /sizeof(state[0]); i++)
  {
    int s = state[i];
    std::cout << s << ": ";

    if (! s&Outside)
      std::cout << "not Outside. ";
    else
      std::cout << "Outside. ";

    if (! s&Inside)
      std::cout << "not inside. ";
    else
      std::cout << "inside. ";

    if (! s&Allocated)
      std::cout << "not Allocated. ";
    else
      std::cout << "Allocated. ";
    std::cout << std::endl;
  }
@vrabaud
Copy link
Contributor

vrabaud commented Jun 25, 2015

Super late on that one but: I agree with your remarks but I believe that the code should be if (! (s&Inside)).
First, well, that's how it reads. Then, it actually does not change anything as you proved: if s is Inside, the two expressions give the same results: (! (1&1))=(!1)=0 and ((!1)&1)=(0&1)=0. If the conditions were true at the lines cppcheck mentions, well, the code would simply stop. If that never happens, that means the check for Inside is done before the function call or does not need to be,
So I'll make the change to get cppcheck quite.

I also agree with you too that it does not make sense for Outside and Allocated but those are not in map.h

vrabaud added a commit to ros-perception/openslam_gmapping that referenced this issue Jun 25, 2015
@vrabaud vrabaud closed this as completed Jun 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants