PLEASE REVIEW: modularize: new preprocessor conditional directive checking feature

[Sean Silva]

On Thu, Jun 20, 2013 at 6:41 PM, Thompson, John <
John_Thompson at playstation.sony.com> wrote:

>  Sean,
>
> Thanks for your efforts in this.  As you can see, I’m struggling to become
> comfortable with this style of coding and using STL, but with your help I’m
> starting to learn.
>

Cool. I'm glad I'm able to help.


> >Again, this function is not appropriate. Please investigate the
> implementation of clang's macro-related diagnostic messages for
> inspiration, or consult with cfe-dev with a more refined question than your
> last question (see below; I think I may have identified what the key
> question is); the appropriate solution will almost surely require
> significant changes to your patch. Clang gives detailed notes in
> diagnostics if the offending code was expanded from a macro, and traces the
> expansion many levels deep in some cases.
>
> I will continue to look into this tomorrow.  Today I just pretty much
> rewrote a lot of the code per your comments about style and conventions.
>
> Previously you mentioned that I should make smaller patches, which implies
> an iterative approach.  Please consider this a step in the process.  The
> bottom line is that even though this approach is flawed, it nonetheless
> mostly works.
>

Iterative doesn't mean committing code that is known to be using a flawed
approach. Iterative means committing the smallest possible piece of code
along the *right* approach. A *huge* part of it is interacting with the
community to gain consensus about the right approach and cleaning up
existing code to make the right approach possible.

Now that I look at Modularize.cpp a bit more closely, one of the first
obvious incremental steps to take is to factor the existing code so that
the checks are cleanly separated. I expect that there are *at least* 5
patches' worth of incremental cleanups and refactoring to be made there.


>   Note that having the preprocessed conditional is primarily for the
> benefit of the user in displaying the message to help him understand which
> macro is different.  An alternative would have been to modify the
> PPCallbacks mechanism to pass along a flag indicating the resulting
> condition Boolean value, rather than my using a string compare.
>

That sounds like a better approach (it sounds like Argyrios had some good
feedback; you probably should continue that discussion instead of leaving
him hanging!). AFAIK PPCallbacks isn't being pushed to give as good of
information as Clang can give you, and there may be significant
improvements to be made to it which would make life a lot easier for
modularize (and other tools, too). E.g. notice that there are some FIXME's
on PPCallbacks that might be useful for modularize.

On the other hand, keeping track of the SourceRange's in SourceRangeSkipped
and ensuring that the same SourceRange's are skipped for a given file might
be an easier approach that doesn't require any modifications to clang.


> The fact that it didn’t pass along this flag seems to be an oversight, but
> I can fix it in a separate patch.  Even though the preprocessed output
> isn’t correct in the case of function macros, because it still incidentally
> likely turns out different when a macro is used or a macro argument is
> passed, it’s still valid as a test for comparing the conditions.
>
> If you find more style and convention problems, please do pass them along
> and I will address them before checking in.  But please let me check this
> in as a functional step in the process.  After all, this is still an
> experimental tool, and I think it would be better to make incremental
> changes, starting with this as a baseline.
>
> Thanks for the reference to clang-format.  It’s made things much easier.
>

I love clang-format :) I also can't wait for clang-tidy which should
similarly automate many of the other stylistic comments that I have made.


> The latest patch is enclosed.
>

There are still numerous issues that I mentioned that are unaddressed, from
variables still named incorrectly to use of fixed-size char buffers and
C-style string manipulation to error-prone memory management. At this point
I recommend taking a step back from your current patch and start by taking
some iterative steps cleaning up and factoring the existing modularize code
as I mentioned above, in preparation for adding new checks.

While I don't claim to be a paragon of incremental development, you may
find it helpful to see the steps I took leading up to the initial ELF
support for yaml2obj, which I think are fairly representative of most new
feature work in LLVM: r183332, r183335, r183711
Notice how the changes to the existing code are tiny and obvious in
r183711, and I only added the new functionality after cleaning up the
existing functionality to ensure that adding the initial version of my
feature was a tiny, incremental step that cleanly meshed with the existing
code organization.
More reading about incremental development: <
http://llvm.org/docs/DeveloperPolicy.html#incremental-changes> (although it
talks about "large/invasive changes", I find that the key ideas are fractal)

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