[PATCH] Add small text file reader facility.

Chandler Carruth chandlerc at gmail.com
Thu Dec 26 20:45:27 PST 2013


  I had a bunch of nit-picky style comments here, as well as some design suggestions, and it just seemed both really confusing and time consuming to try to put all of them here in the review.

  Instead, I grabbed the patch, and hacked up the code some to try to convey the idea for how to factor this. I've committed it in r198068 so we don't have to start swapping and merging patches, but I've not wired it into the profile reading or anything so we can iterate on it in-tree if its not going to work.

  The primary changes I was trying to suggest were: 1) separating the concerns of creating a valid memory buffer and iterating over the lines, and 2) using the more standard-library style of interface due to this being an iterator-ish thing. Lots of little changes required there.

  I was also able to write some more tests more easily because it wasn't tied to a file.

  Finally, I ended up factoring the code a bit differently to reduce the number of methods involved. I found this simplified it a lot for me, but maybe it doesn't work well for you? Not sure.

  Anyways, please comment on the revision and/or make tweaks to it until it suits you, and hopefully that will serve as an easy basis. I've left some comments on the parsing code, but I don't know that those need to be fully addressed before you commit. Feel free to get the profile loading and parsing code into a shape that you're happy with (it's already pretty mechanical) and just commit -- that part of the change LGTM once we sort out the iterator business.



  PS: For some bad reason I looked at the code that LLVM generated for this class, and I got really sad. I fiddled with how the looping was written a bit to no avail. I'm going to file some bugs. While the performance of this thing couldn't possibly matter, we at least shouldn't get simple things dead wrong. =[


================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:366
@@ -417,3 +365,3 @@
 void SampleModuleProfile::loadText() {
-  ExternalProfileTextLoader Loader(Filename);
+  FileReader Loader = FileReader::begin(Filename, '#');
 
----------------
I would suggest "LineIt" or something for the name maybe?

================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:372-379
@@ -424,7 +371,10 @@
+                     "Expected 'symbol table', found " + *Loader);
+  ++Loader;
   int NumSymbols;
-  Line = Loader.readLine();
-  if (Line.getAsInteger(10, NumSymbols))
-    Loader.reportParseError("Expected a number, found " + Line);
-  for (int I = 0; I < NumSymbols; I++)
-    Profiles[Loader.readLine()] = SampleFunctionProfile();
+  if ((*Loader).getAsInteger(10, NumSymbols))
+    reportParseError(Loader.getLineNumber(),
+                     "Expected a number, found " + *Loader);
+  ++Loader;
+  for (int I = 0; I < NumSymbols; ++I, ++Loader)
+    Profiles[*Loader] = SampleFunctionProfile();
 
----------------
No where here do you check for reaching the end of the file... Do you want to?


http://llvm-reviews.chandlerc.com/D2294



More information about the llvm-commits mailing list