[PATCH] Add small text file reader facility.

Diego Novillo dnovillo at google.com
Mon Dec 2 08:53:29 PST 2013


On Fri, Nov 29, 2013 at 7:37 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
>
>   I like factoring this out, but I think it needs a different interface. Specifically, I think that it should be structured more as a line iterator over a file. This could look much like many of our stateful iterators, for example in SCCIterator.h. The iterator can still expose methods for getting the line number.

Hm, would it make more sense to add a line-iterator type to
MemoryBuffer?  This is what this class does, after all.

>   I have some more minor comments below on the API that will likely carry over...
>
>
> ================
> Comment at: include/llvm/Support/FileUtilities.h:96
> @@ +95,3 @@
> +  public:
> +    FileReader(StringRef F, const char Marker = '\0')
> +        : Filename(F), CommentMarker(Marker) {
> ----------------
> I would accept a twine for the filename so that they can be easily composed when calling this.

Done.

> ================
> Comment at: include/llvm/Support/FileUtilities.h:146-147
> @@ +145,4 @@
> +
> +    /// \brief Path name to the file being read.
> +    StringRef Filename;
> +
> ----------------
> I think you want to store the filename so that users can provide temporaries without fear.

Done.

> ================
> Comment at: include/llvm/Support/FileUtilities.h:128-129
> @@ +127,4 @@
> +
> +    /// \brief Return the current line number.
> +    int64_t getLineno() const { return Lineno; }
> +
> ----------------
> I think 'Lineno' is not a great abbreviation. I would just spell it out as 'getLineNumber'

Done.

> ================
> Comment at: include/llvm/Support/FileUtilities.h:131-134
> @@ +130,6 @@
> +
> +    /// \brief Report a parse error message and stop compilation.
> +    void reportParseError(Twine Msg) const {
> +      report_fatal_error(Filename + ":" + Twine(Lineno) + ": " + Msg + "\n");
> +    }
> +
> ----------------
> I'm inclined to sink this code into the user of this library. Essentially, make sure they can extract the filename and line number from the interface, and then the user can call report_fatal_error directly?

Done.




More information about the llvm-commits mailing list