Issue# 168: Fix invalidHashHash exception on valid token.

[bedw021]

Issue: #168

[bedw021]

Err.. suppose ''' is a character constant not covered by the rule it's defining 😆

[danmar]

I am not sure we should generate such tokens. If a token starts with a ", I believe it will be classified as a string by cppcheck.

How about outputting something like operator_y("A") .. 4 tokens?

There was some tweak in Cppcheck for these operators a while ago I am not sure how that works. Can you double check? It would be good if we match that. Define such operator and look at the debug output cppcheck --debug.

[bedw021]

I copied these changes to simplecpp in cppcheck and ran cppcheck on the below file:

example.cpp

#include <cstdlib>

#define NUM_CHAR(str) str##_numch
#define X(str) str##_x
#define Y(ch) ch##_y
#define Z(val) val##_z

inline auto operator "" _x(const char *ch, std::size_t size) noexcept -> std::size_t
{
  return size;
}

inline auto operator "" _y(char ch) noexcept -> double
{
  return static_cast<double>(ch) * 0.25;
}

inline auto operator ""_z(long double val) noexcept -> const char *
{
  return (val < 0.5) ? "ABC" : "EFG";
}

int main()
{
  auto a = "abc"_x;
  auto b = X("abc");
  auto c = 't'_y;
  auto d = Y('t');
  auto e = 1.0_z;
  auto f = Z(1.0);

  auto g = operator"" _x( "abc", 3);
  auto h = operator"" _x("abc"); // Wrong signature
}

As you thought, the change caused an error to be generated by cppcheck. I removed the macros and ran it again to see what it produces normally, and made the following further change to try to mimic how it appeared to be handling them:

// @line 1842:
if (isStringConstant || isCharConstant)
  output->push_back(new Token(*B));
else if (varargs ... //etc

With this change, cppcheck runs and does not produce an error. Some select output from --debug and --verbose below:

1:
|
7:
8: auto operator""_x ( const char * ch@1 , unsigned long size@2 ) noexcept . unsigned long
9: {
10: return size@2 ;
11: }
12:
13: auto operator""_y ( char ch@3 ) noexcept . double
14: {
15: return static_cast < double > ( ch@3 ) * 0.25 ;
16: }
17:
18: auto operator""_z ( long double val@4 ) noexcept . const char *
19: {
20: return ( val@4 < 0.5 ) ? "ABC" : "EFG" ;
21: }
22:
23: int main ( )
24: {
25: auto a@5 ; a@5 = "abc" _x ;
26: auto b@6 ; b@6 = "abc" _x
5:
|
25:
26: ;
27: auto c@7 ; c@7 = 't' _y ;
28: auto d@8 ; d@8 = 't' _y
6:
|
27:
28: ;
29: auto e@9 ; e@9 = 1.0_z ;
30: auto f@10 ; f@10 = 1.0_z ;
31:
32: auto g@11 ; g@11 = operator""_x ( "abc" , 3 ) ;
33: auto h@12 ; h@12 = operator""_x ( "abc" ) ;
34: }

##AST
(
|-operator""_x
`-,
  |-*
  | |-char
  | `-ch 'const signed char *'
  `-size 'unsigned long'

return ''
`-size 'unsigned long'

(
|-operator""_y
`-ch 'signed char'

return ''
`-* 'double'
  |-( 'double'
  | |-static_cast
  | `-ch 'signed char'
  `-0.25 'double'

(
|-operator""_z
`-val 'long double'

*
|-.
| |-noexcept
| `-char
`-{

( 'signed int'
`-main

= 'const char *'
|-a 'const char *'
`-"abc" 'const char *'

= 'const char *'
|-b 'const char *'
`-"abc" 'const char *'

= 'char'
|-c 'char'
`-'t' 'char'

= 'char'
|-d 'char'
`-'t' 'char'

=
|-e
`-1.0_z

=
|-f
`-1.0_z

=
|-g
`-(
  |-operator""_x
  `-,
    |-"abc" 'const char *'
    `-3 'signed int'

=
|-h
`-(
  |-operator""_x
  `-"abc" 'const char *'

From that it looks like just splitting it into two tokens and discarding the ## is doing the same as cppcheck is right now. It seems cppcheck is mostly just ignoring these operators right now. It has at least got the type wrong for all cases.

Should we go ahead with this to make it like how cppcheck processes it, or try operator"" {TokenB} ( {TokenA} ) which might give more meaningful output in cppcheck (although it still doesn't seem to get the return type from the function).?

[danmar]

Thanks! That is a good investigation. I feel much more comfortable now. :-)

Should we go ahead with this to make it like how cppcheck processes it, or try operator"" {TokenB} ( {TokenA} ) which might give more meaningful output in cppcheck (although it still doesn't seem to get the return type from the function).?

I suggest the first option for now. We need to handle this better in cppcheck.

[danmar]

you should probably check what tokens you get if you don't use ## also :

 a = 1_y;
 b = "x"_y;

[BillDenton]

How will this roll out into a release of cppcheck? I've just encountered this problem which for now I have inline suppressed.

[danmar]

I am anxious to merge this. I just want to know that it really works. As I wrote.. I believe @IOBYTE made some fix in Cppcheck for this C++ feature a while ago.. does this match that fix?

How is this output handled by Cppcheck? I believe that "x"_y shouldn't be treated as a string. What is the type set by Cppcheck? You can see that by using --debug --verbose and look in the AST output.

[bedw021]

Sorry, I forgot about this. I'll try to look into it this weekend. Thanks for the pointers.

[danmar]

great!

[BillDenton]

@bedw021 Has this fixed been merged into simplecpp? Any idea if it has then made its way into CppCheck? Thanks.

[danmar]

I have the feeling we have not got a fix in simplecpp yet. :-(

If anybody wants to take over and work on this .. I think that is possible.

[BillDenton]

@danmar @bedw021 Could this be merged into simplecpp so that CppCheck can be updated? We've got inline suppresion for CppCheck in our codebase due to this. Thanks.

[danmar]

@BillDenton feel free to finish this. the comment @bedw021 made on aug 15th sounds pretty promising to me.

[danmar]

On Aug 15th I wrote:

Should we go ahead with this to make it like how cppcheck processes it, or try operator"" {TokenB} ( {TokenA} ) which might give more meaningful output in cppcheck (although it still doesn't seem to get the return type from the function).?

I suggest the first option for now. We need to handle this better in cppcheck.

@BillDenton feel free to implement first option. Or if you prefer option 2 that is fine but then you'll need to tweak cppcheck also.

[danmar]

@BillDenton I don't know this feature very well. I have never used it myself. Could you write some minimal "compilable" example(s) that I can try out?

Ideally simplecpp should output the "proper" output. And then if that is not ideal for Cppcheck then we should fix Cppcheck.

[patrickdowling]

Third time’s the charm? I also stumbled into a/the exception with macro + operator"" constructs so I guess a fix is still pending?

With a similar set of (naive) changes hacked into my local version (current from a few days ago) it looks plausible for simple test cases at least (“works for me”). I think there may be cases where B needs expanding but building test cases is always bit of a rabbit hole...

Are there similar tickets over on the cppcheck trac?

[danmar]

@patrickdowling sorry for late reply. yes it would be good to get this fixed finally.
I am not sure if there is corresponding tickets in cppcheck. I don't think so.

[patrickdowling]

@danmar No worries. So the I’ve somewhat brute forced things in this branch with some dangling todos and have a few simplecpp vs. g++ scenarios here. On the way I’ve realised how hazy I am on the details of macros/pasting so I'm open to corrections.

Perhaps it makes sense to roll in other ## related issues? E.g. as has been suggested it’d be nice to see what is not being pasted when it fails by extending struct invalidHashHash, but I'm also wary of scope creep 😀 Should I open a fresh PR and move the discussion there?

[danmar]

I'm also wary of scope creep

yes me too. And this has been open long enough already. I suggest you try to solve just a specific problem now and we can look at related issues later. I think your suggestion to extend invalidHashHash sounds good for instance.