On Thu, Jun 20, 2013 at 6:41 PM, Thompson, John < [email protected]> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
