[clang] [clang][analyzer] Improved message of CallAndMessageChecker at calls with uninitialized argument (PR #211038)

[Donát Nagy via cfe-commits]

================
@@ -30,7 +30,7 @@ extern time_t mktime(struct tm *timeptr);
 
 void uninit_mbrlen(const char *mbs) {
   mbstate_t state;
-  mbrlen(mbs, 1, &state); // expected-warning{{3rd function call argument points to an uninitialized value}}
+  mbrlen(mbs, 1, &state); // expected-warning{{3rd function call argument points to an uninitialized value; Function 'mbrlen' expects memory pointed to by this argument to be initialized}}
----------------
NagyDonat wrote:

> Did we have a precedent for having a semicolon in the diagnostic message? 

There are many precedents, e.g.
- Property of mutable type 'NSMutableString' has 'copy' attribute; an immutable object will be stored instead
- Opened file is never closed; potential resource leak
- Return is prohibited after a successful vfork; call _exit() instead

However, the word after the semicolon should start with a lowercase letter -- it is not a separate sentence.

> I wonder if regular punctuation signs would fit better here, like a full-stop.

There are also precedents for using a full-stop, but I think it is really ugly to end a sentence with a full stop, and have a second sentence after it where the full stop (which would be at the end of the message) is omitted.

> Alternatively, we can always emit a note alongside the warning location.

That would be also acceptable for me, but I prefer including this in the warning message to ensure that the warning message is understandable on its own

> @NagyDonat Isn't that the rule of thumb is that the warning messages should be plain-old strings - not patterns.

I don't think that there is such rule of thumb, and in fact I'm frequently writing checkers where the warning message varies dynamically.

> Because having parametric warning messages break uniqueing? Warnings with different messages are treated as separate warnings - regardless if they have the same BugType and everything else.

The clarification added by this PR cannot break uniqueing, because the added string will always be the same for two bug reports at the same location. (The source location is already counted as a distinguishing feature in all bug reports except those that explicitly set a different uniqueing location, e.g. the allocation point of the leaked resource.)  

--------

Also note that there is the `ShortDescription` / `Description` distinction which is IMO not directly relevant here, but is important to to know for similar situations:

A `BugReport` has __two__ data members that store diagnostic messages: `ShortDescription` (the `warning` diagnostic) and `Description` (the `note` level diagnostic at the end of the bug path). By default these are the same [1], but `BugReport` (and `PathSensitiveBugReport`) has constructors that allow specifying them separately.

The uniqueing process includes the `ShortDescription` in the identity of the bug report, but ignores the `Description`. (Reports with different `ShortDescription` are treated as separate reports, while reports that only differ in the `Description` are treated as identical -- i.e. the user will only see one of them, while the rest are discarded.)

[1] Implementation detail: technically the default behavior is that `ShortDescription` is the empty string and in this case `getShortDescription()` will return the `Description`.

https://github.com/llvm/llvm-project/pull/211038