[llvm-commits] RFC: new tool: llvm-strings
Sean Silva
silvas at purdue.edu
Wed Nov 14 14:40:40 PST 2012
+ if (isPrintable(*p)) {
+ if (curr == NULL)
+ curr = p; // start new string
+ }
can these be collapsed into one test? Also, locals are LikeThis
(please fix throughout) [1].
+ if (sz >= sizeThreshold)
+ if ((RetVal = pf(initialOffset + (curr - theData.begin()),
+ StringRef(curr, sz))))
+ break;
align the S of `StringRef` under the i of `initialOffset`.
+ break;
+ curr = NULL; // start a new string
+ }
+ }
+
+ return RetVal;
just return RetVal instead of break'ing
+ OwningPtr<MemoryBuffer> aFile;
This name is weird. `MB` or `Buffer` sounds more idiomatic.
+ if ((RetVal = MemoryBuffer::getFileOrSTDIN(main_fn, aFile)))
+ errs() << "## Can't open file '" << main_fn << "'!" << endl;
use an early return here [2].
+ if (OptionVerbose)
+ outs() << "Processing '" << main_fn << "' as a binary file" << endl;
+ RetVal = findStringsInBlob(aFile->getBuffer(), 0,
+ OptionMinStringLength, OptionVerbose, printFn);
+ }
please use early returns all over this function
+ if (!(RetVal = object::createBinary(aFile.take(), obj)))
+ if (object::ObjectFile *o = dyn_cast<object::ObjectFile>(obj.get()))
+ RetVal = findStringsInObjectFile(o, 0, OptionMinStringLength,
+ OptionAllObjectSections, OptionVerbose, printFn);
same here. something like
if (error_code Err = object::createBinary(aFile.take(), obj)))
return Err;
if (object::ObjectFile *o = dyn_cast<object::ObjectFile>(obj.get()))
return findStringsInObjectFile(o, 0, OptionMinStringLength,
OptionAllObjectSections, OptionVerbose,
printFn);
return error_code();
+ else if (*fileName.rbegin() == ')') { // Looking for "xxx(yyy)"
are you sure that !fileName.empty()?
+ if (fileName == "-")
+ main_fn = "<stdin>";
MemoryBuffer::getFileOrSTDIN() already handles "-" [3]. I'm not sure,
but my guess is that "<stdin>" will confuse it.
+error_code processOne(const std::string &fileName) {
+ error_code RetVal;
+ OwningPtr<MemoryBuffer> aFile;
+ std::string main_fn = fileName;
+ std::string sub_fn;
declare at or near the point of use.
+ if (OptionVerbose)
+ outs() << "Processing '" << main_fn << "' as a binary file" << endl;
how about `verboseOS() << "Processing '" [...]`
and have
raw_ostream &verboseOS() { return OptionVerbose ? outs() : nulls(); }
+ sub_fn = fileName.substr(open_paren + 1,
+ fileName.size() - open_paren - 2);
Align the 'f' to be under the 'o' (and rename the variables to
OpenParen, FileName).
+ if (!Iter->getAsBinary(child)) {
+ if (object::ObjectFile *o = dyn_cast<object::ObjectFile>(child.get())) {
+ if (verbose) {
+ outs() << "Archive section: '" << o->getFileName() << "'" << endl;
+ }
+
+ if (onlyFile.empty() || onlyFile == o->getFileName()) {
+ std::size_t childOffset = initialOffset; /* + something */
+ RetVal = findStringsInObjectFile(o, childOffset,
+ sizeThreshold, dumpAllSections, verbose, pf);
+ FoundIt = true;
+ }
+ }
+ }
+ }
use early return's/continue's here to reduce the nesting. You can
probably remove the test !RetVal from the loop condition too (test
RetVal the moment you get it).
> Now I just need docs and tests.
For docs, you will probably find
<http://www.llvm.org/docs/SphinxQuickstartTemplate.html> useful.
However, you will want to copy the format and organization of the
other "manpages" in docs/CommandGuide/.
[1]. http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
[2]. http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
[3]. http://llvm.org/docs/doxygen/html/classllvm_1_1MemoryBuffer.html#a5997e214c757f7c378223049a442233b
On Wed, Nov 14, 2012 at 3:30 PM, Marshall Clow <mclow.lists at gmail.com> wrote:
> On Nov 13, 2012, at 12:40 PM, Sean Silva <silvas at purdue.edu> wrote:
>> On Tue, Nov 13, 2012 at 10:29 AM, Marshall Clow <mclow.lists at gmail.com> wrote:
>>> On Nov 12, 2012, at 12:58 PM, Eric Christopher <echristo at gmail.com> wrote:
>>>
>>> On Mon, Nov 12, 2012 at 7:58 AM, Marshall Clow <mclow.lists at gmail.com>
>>> wrote:
>>>>
>>>> Here's a first cut at a "strings" tool for LLVM.
>>>>
>>>> It is currently missing one feature: the ability to dump the strings from
>>>> a single object file in an archive.
>>>> There are also some 80 column style violations in the source.
>>>>
>>>> I'll be fixing both of those and re-posting soon - but I'm posting now
>>>> this to get feedback - to see if anyone thinks that I'm missing (any other)
>>>> stuff.
>>>
>>>
>>> Tabs, oh god the tabs.
>>>
>>>
>>> To spare Eric the agony, I have:
>>> * removed all the tabs
>>> * cleaned up a bunch of style violations
>>> * implemented the feature to dump only the contents of one object file in an
>>> archive.
>> +error_code findStringsInBlob(StringRef theData, std::size_t initialOffset,
>> + std::size_t sizeThreshold, bool verbose, PrintFunction pf) {
>>
>> Please align this like the rest of the codebase.
>>
>> +// outs() << "Blob( " << theData.size() << ", " << initialOffset << ", "
>> +// << sizeThreshold << ", pf)\n";
>>
>> Don't leave commented-out code in your patches. Remove before submitting.
>>
>> + for ( ; p != end && !retVal; ++p ) {
>>
>> check your spacing around parens to be like the rest of the codebase.
>>
>> + if ( isPrintable( *p )) {
>> + if ( curr == NULL )
>> + curr = p; // start new string
>> + }
>>
>> Please put your curly braces like the rest of the codebase and not
>> this bizarre "indent the end brace" thing you are doing.
>>
>> +// Display the current settings, if asked
>> + if ( OptionVerbose ) {
>>
>> The comment should be at the same indentation as the code it comments.
>
>
> Ok, I think I've got all the style guidelines.
> I haven't heard any complaints about the code ;-)
>
> Now I just need docs and tests.
>
>
> -- Marshall
>
> Marshall Clow Idio Software <mailto:mclow.lists at gmail.com>
>
> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait).
> -- Yu Suzuki
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list