[PATCH] Remove PragmaIntroducerKind?

[Thompson, John]

If we ever want to go back to using the unittest, I added to the existing PPCallbacks unit test a generic test for testing the PPCallbacks callbacks here: http://llvm-reviews.chandlerc.com/D1966

This does pretty much what the pp-trace test does, except that it reads the comparison master data from an internal string array rather than a file.  Sean Silva pointed out that creating a tool that displays PPCallbacks information could be useful, and also be that basis for testing it.  Kind of like hitting two birds with one stone.

Kim suggested that because it tests a Clang mechanism, pp-trace and it's tests should probably be moved to the Clang tree proper.  I pinged this list a couple of times asking if I could move it, but didn't get any feedback.  I started working on it anyway, but then chickened out due to the lack of feedback, and then left on vacation.  Now back, I got caught up in something else (a new tool for checking module.map coverage), so I let it hang for now.

I think Kim has a point (about moving pp-trace), but it would be nice if someone else chimed in too.  I don't feel strongly either way, as I believe the "extra" tests do get run frequently, and someone in another thread said that they should be run by developers before checking in related changes.  But there's a chance that someone might change the preprocessor or PPCallbacks and not do that, leading to a build breakage.

Putting in my changes to the PPCallbacks unit test is an option, but probably doesn't make sense as it fairly closely overlaps the pp-trace tests.  But since neither of these tests deeply test the arguments passed to the PPCallbacks callback functions (I generate strings to represent the arguments, but as some arguments are data structures, I only picked one or two identifying aspects, or even just a non-null indicator for some), someone might make the argument that a deeper test is needed, which would either involve writing a better test in the PPCallbacks unit test, or extending pp-trace.

As an aside, it seems that PPCallbacks coverage for pragmas is missing support for a fair number of the pragmas.  Also, it seems that it would be nicer if there were a generic callback for all pragmas that gave you all the needed information about which pragma it was, and all the arguments, rather than specific callbacks for each pragma, but perhaps it wasn't feasible.  The HandlePragma callback doesn't do this.

So, if someone wants to chime in about moving pp-trace from extra to clang, please do so.

And please remember that if you make changes to PPCallbacks or the preprocessor's use of it, to please update pp-trace and it's tests too.

Thanks.

-John

-----Original Message-----
From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Kim Gräsman
Sent: Saturday, December 14, 2013 2:13 AM
To: Renato Golin
Cc: Clang Commits; John Thompson
Subject: Re: [PATCH] Remove PragmaIntroducerKind?

Hi Renato,

On Wed, Dec 11, 2013 at 4:34 PM, Renato Golin <renato.golin at linaro.org> wrote:
>
> I think the problem here is that there isn't a single line in Clang (I 
> could find) that uses that information, so it's impossible to create a 
> test that doesn't involve external code.
>
> If we had a dumper that used this code in some standard way, we could 
> use that as both to test and as a documentation on how to use it (for 
> the external users).

John Thompson built a tool like that a few weeks back:
https://github.com/llvm-mirror/clang-tools-extra/tree/master/pp-trace

for the specific purpose of testing PPCallbacks. He's been trying to call attention to getting it into Clang core.

I'm still on the fence whether it's a good idea to build a separate tool to test PPCallbacks, because failures would signal problems with the tool, not with PPCallbacks. Intuitively, I think unit tests would be better, but that was discouraged by someone else in the original review.

FWIW,
- Kim
_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits