PLEASE REVIEW: modularize: new preprocessor conditional directive checking feature

[Sean Silva]

I recommend looking at the code and trying to break it down into a bunch of
small, "obvious", incremental changes to the existing code. It's really
hard to review such a huge patch.

On Wed, Jun 12, 2013 at 1:41 PM, Thompson, John <
John_Thompson at playstation.sony.com> wrote:

>  Thanks a bunch, Sean.****
>
> ** **
>
> I’ve cleaned up the sources a lot per your comments, but I still need to
> replace the macro substitution function, which will mean I can do away with
> the symbol storing too.  I couldn’t find a function such as I need, i.e. a
> function in Preprocessor that will either take a string or source range as
> input and produce either a string or token vector, with macro substitutions
> done using the current preprocessor state, all without adversely affecting
> the preprocessor state.  But it’s a huge class, so I might be missing it.
> Do you or anyone know of such a function?  Otherwise, I’ll work on a
> separate patch to add one to Preprocessor.
>
Not off the top of my head. You may want to ask on cfe-dev for the best way
to approach this.

> Also, in addition to the macro substitution, I think I can relatively
> easily add a similar message to the “header (file) has different contents
> depending on how it was included” error, relating the error to the
> differing preprocessor conditional.  Likewise, an option for showing the
> include hierarchy would help.****
>
> ** **
>
> Anyway, may I check this in as a working intermediate step, in case folks
> find it useful?****
>
> **
>
Did you send the right patch? You don't seem to have addressed many of my
review comments. Please revisit my last message and make sure that you have
addressed those issues. There are numerous other changes I would like to
suggest but the ones I pointed out earlier need to be fixed before the
other changes can be meaningfully discussed.


-- Sean Silva
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130612/9d5cae75/attachment.html>