warning for meaningless class combinations

[YashasSamaga]

#172

1. Adds a generic new warning which is triggered when meaningless combination of class specifiers are used.

2. This commit adds code to trigger a warning for the following two cases:

• constant reference
• • not meaningful as as references always point to cells in PAWN (unlike C++ where it would help prevent costly copies while guaranteeing to not modify the object) which makes const reference as good as pass-by-value
• constant variable arguments
• • for similar reasons as in the previous case

[Zeex]

Can you also add a test? It can be a simple compilation test that checks if the warnings are printed correctly (see source/compiler/tests). Maybe optionally check valid combinations of specifiers so that some future change doesn't break them by accident.

[YashasSamaga]

I have added the tests in a separate branch to prevent the commits from polluting this PR with bad commits. I'll move them here in a single commit once I'm sure that I'm doing this right.

Test script: YashasSamaga@52b97f6

CMakeLists.txt change: YashasSamaga@b3ed64b

Is that how it is supposed to be?

1. is meaningless_class_specifiers_warnings_246.pwn over-verbose?
2. what does gh mean?
3. should the issue number be used or the PR number to be used to name the test (and the file)?
4. any other suggestions?

[Y-Less]

Could you add a test for warning pop/disable/push around it please? Just to ensure it will work in the very rare case where it is desired. Thanks.

[Zeex]

gh means GitHub, so gh_246 would be "GitHub issue 246". You can name it whatever you want, just make sure that the issue number is included in the name so it's easy to look up the corresponding issue.

[YashasSamaga]

@Y-Less Those directives work independently of the commits in this PR. The commits in this PR use the error function to trigger the warnings. Whether to trigger the warning or not (because it is enabled or disabled, or if the compiler is in first pass or second pass, or whatever the reason maybe) is decided by the error function which this PR does not modify. Therefore, I think the tests you suggested are not really relevant to this PR.

As one could expect, the directives will work with this warning too:

#pragma warning push
#pragma warning disable 238
f1(const &v) { }
#pragma warning pop
f2(const ...) { }
f3(const v) {
	#pragma unused v
}
f4(...) { }
f5(v) {
	#pragma unused v
}
f6(&v) { }

main() {
	new a;
	f1(a);
	f2(a);
	f3(a);
	f4(a);
	f5(a);
	f6(a);
}

Output:

warning 238: meaningless combination of class specifiers (const variable arguments)

[Zeex]

Test name should refer to the bug/issue number, not the PR number since it tests that there is no bug, not that the PR works. If a PR adds some new feature I wouldn't add its number to the name at all.

It could be useful to know where is the original bug report on GitHub that a test addresses, but not as much useful to know which PR implemented the fix (or a feature). It could also be multiple PRs that address the same problem. Well, I don't know, it's kind of hard to explain, maybe I'm overcomplicating things and everybody should just name tests whatever they want.... I didn't do a lot of automated tests in the past.

Just use a descriptive name that makes sense and is short enough to fit on screen.