[PATCH] D48850: [GISel]: Testing LegalizerHelper using Unit tests

[Justin Bogner via Phabricator via llvm-commits]

bogner added inline comments.


================
Comment at: include/llvm/Support/FileCheck.h:1
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
----------------
bogner wrote:
> Please go through these and see which ones are still needed in the header. I think some of these are only needed in the .cpp file now.
This is missing the standard llvm header and include guards.


================
Comment at: include/llvm/Support/FileCheck.h:1-14
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Regex.h"
+#include "llvm/Support/SourceMgr.h"
----------------
Please go through these and see which ones are still needed in the header. I think some of these are only needed in the .cpp file now.


================
Comment at: include/llvm/Support/FileCheck.h:51
+
+class FileCheckPattern {
+  SMLoc PatternLoc;
----------------
These top level structs, enums, and classes could probably all use brief doxygen comments about what they're for.


================
Comment at: include/llvm/Support/FileCheck.h:88
+
+  bool ParsePattern(StringRef PatternStr, StringRef Prefix, SourceMgr &SM,
+                    unsigned LineNumber);
----------------
It's probably cleaner to do in a separate patch (either before or after), but we might as well change these names to fit the modern llvm convention of starting with a lower case letter since we're making them public.


https://reviews.llvm.org/D48850