[PATCH] D20858: [clang-format] make header guard identification stricter in header insertion.

Eric Liu via cfe-commits cfe-commits at lists.llvm.org
Wed Jun 1 06:11:43 PDT 2016


ioeric added inline comments.

================
Comment at: lib/Format/Format.cpp:1472
@@ -1471,2 +1471,3 @@
   llvm::Regex IncludeRegex(IncludeRegexPattern);
-  llvm::Regex DefineRegex(R"(^[\t\ ]*#[\t\ ]*define[\t\ ]*[^\\]*$)");
+  llvm::Regex IfNDefRegex(R"(^[\t\ ]*#[\t\ ]*ifndef[\t\ ]*([^\\]*)$)");
+  llvm::Regex DefineRegex(R"(^[\t\ ]*#[\t\ ]*define[\t\ ]*([^\\]*)$)");
----------------
djasper wrote:
> I think there is too much in the regex group here. If there are trailing comments on the #ifndef or #define lines, this should still work. Also, there might actually be empty lines or comment lines in-between the two. I would for now just check that there:
> - Is no non-comment line before the first #ifndef
> - That there is a #ifndef
> - That the next non-comment line after the #ifndef is a #define
> 
> We can refine more if necessary.
> 
> I think we should be conservative here as this should only really ever affect .cc files (or else the header guard would be there and identified correctly) that don't have any #includes (or put them after unrelated #defines). So, we have to weigh this case being so rare against incorrectly identifying weird header guards in headers (which IMO might be more common).
Okay.

I guess I should've work on the comment skipping FIXME first. I'll work on skipping comments first and then continue with this patch,


http://reviews.llvm.org/D20858





More information about the cfe-commits mailing list