added partial support for -p flags in lsbom #207

brtsai · 2016-12-05T07:54:41Z

No description provided.

grp · 2016-12-05T07:55:50Z

Libraries/libbom/Tools/lsbom.cpp

+ *      Username
+ *      UserGroupName
+*/
+std::string


nit: static std::string

grp · 2016-12-05T07:57:36Z

Libraries/libbom/Tools/lsbom.cpp

+ *      UserGroupName
+*/
+std::string
+printItemToString(Options::PrintItem item, std::string path, struct bom_path_info_2 *info){


nit: PrintItemToString (or StringFromPrintItem)
nit: { on a new line

grp · 2016-12-05T07:57:45Z

Libraries/libbom/Tools/lsbom.cpp

+*/
+std::string
+printItemToString(Options::PrintItem item, std::string path, struct bom_path_info_2 *info){
+


nit: remove empty line

grp · 2016-12-05T07:58:25Z

Libraries/libbom/Tools/lsbom.cpp

+            return std::to_string(ntohl(info->group));
+        case Options::PrintItem::GroupName:
+
+        break;


For these unfinished ones, I'd do it like this:

case Options::PrintItem::SomeCase: return std::string(); // TODO: implement

grp · 2016-12-05T07:59:25Z

Libraries/libbom/Tools/lsbom.cpp

+        break;
+    }
+
+    return "";


(Once the unimplemented ones have return above, can remove this.)

grp · 2016-12-05T07:59:36Z

Libraries/libbom/Tools/lsbom.cpp

        if (options->onlyPath()) {
            printf("%s\n", path.c_str());
+        } else if (options->printFormat()) {
+


nit: remove empty line

grp · 2016-12-05T07:59:51Z

Libraries/libbom/Tools/lsbom.cpp

+            std::string toPrint;
+
+            for (Options::PrintItem item : *options->printFormat()) {
+                toPrint += first? "" : "\t";


nit: space before ?

brtsai · 2016-12-05T08:20:25Z

grp · 2016-12-06T07:26:32Z

Libraries/libbom/Tools/lsbom.cpp

+    toReturn.push_back(Options::PrintItem::FileSize);
+    toReturn.push_back(Options::PrintItem::Checksum);
+
+    return toReturn;


This could use an "initializer list" to create the vector:

std::vector<Options::PrintItem> toReturn = { Options::PrintItem::FileName, Options::PrintItem::Permissions, Options::PrintItem::UserGroupID, Options::PrintItem::FileSize, Options::PrintItem::Checksum, }; return toReturn;

Or even remove the temporary variable completely:

return { Options::PrintItem::FileName, Options::PrintItem::Permissions, Options::PrintItem::UserGroupID, Options::PrintItem::FileSize, Options::PrintItem::Checksum, };

I like that style better for two reasons. Most importantly, I think it's easier to read: the structure of the code matches the structure of the data structure created. And not important in this case, but it's also slightly faster since it avoids calling push_back five times, each of which might need to expand the vector's allocated space.

The initialiser list looks like a slightly faster, cleaner way to do this.
I'll make that change, thanks!

grp · 2016-12-06T07:29:46Z

Libraries/libbom/Tools/lsbom.cpp

-            }
-
-            printf("\n");
+            toPrint = PrintItemsToString(GetDefaultPrintFormat(), path, path_info_2_value);


Rather than the else if and else, here, you could do this:

} else { std::vector<Options::PrintItem> printFormat = options->printFormat().value_or(GetDefaultPrintFormat()); toPrint = PrintItemsToString(printFormat, path, path_info_2_value); }

Using the built-in value_or function to use a default if no printFormat is specified in the options.

Beyond that, you could even remove the if entirely:

std::vector<Options::PrintItem> printFormat = options->printFormat().value_or( options->onlyPath() ? { Options::PrintItem::FileName } : GetDefaultPrintFormat()); toPrint = PrintItemsToString(printFormat, path, path_info_2_value);

Then, you could remove the PrintItemsToString function and just put the code right in here, since it's only called once.

I notice that in your value_or solution, if printFormat has been filled in via the -p flag, then onlyPath no longer takes precedence over the -p flag as lsbom.
Perhaps you meant:
std::vector<Options::PrintItem> printFormat = options->onlyPath() ? std::vector<Options::PrintItem>({Options::PrintItem::FileName}) : options->printFormat().value_or(GetDefaultPrintFormat());

And this too looks good!
I'll get this in there, and thanks again for the feedback!

Good catch, sounds best to ignore the second half of my comment!

grp · 2016-12-06T07:30:15Z

Libraries/libbom/Tools/lsbom.cpp

+}
+
+static std::vector<Options::PrintItem>
+GetDefaultPrintFormat ()


nit: no space before ()

refactored code for printing lsbom details

brtsai · 2017-01-29T04:06:41Z

Tried pulling a fresh copy from the current main version, and noticed at lsbom doesn't seem to work anymore as I get "error: failed to load paths tree."

Tried tracing it to see what went wrong and here's what I got so far:

bom.c::bom_variable_iterate returned get_context.data_index == -1

	before bom.c::bom_variable_get returned -1

		before bom_tree.c::bom_tree_alloc_load returned null

			before lsbom:: under comment Create Bom Tree for paths errored with “failed to load paths tree error”

grp · 2017-01-31T03:05:29Z

@brtsai I think #225 will fix that

brtsai · 2017-02-06T09:48:28Z

Thanks!
225 did it!
I've merged the latest ver into my repo, added my changes and pushed.
All of the Travis tests seem to have passed and all of the changes posted in your review should be implemented.
Let me know if there's anything I've missed.

facebook-github-bot added the CLA Signed label Dec 5, 2016

grp suggested changes Dec 5, 2016

View reviewed changes

brtsai force-pushed the partial-p-flags branch from 79a1a39 to d10956e Compare December 5, 2016 08:18

brtsai force-pushed the partial-p-flags branch from d10956e to 4cd9560 Compare December 6, 2016 07:12

grp reviewed Dec 6, 2016

View reviewed changes

brtsai force-pushed the partial-p-flags branch from 4cd9560 to 2f239d8 Compare December 6, 2016 08:02

brtsai force-pushed the partial-p-flags branch from 2f239d8 to caf7ede Compare December 21, 2016 17:39

added partial support for -p flags in lsbom

17d96d9

refactored code for printing lsbom details

brtsai force-pushed the partial-p-flags branch from caf7ede to 17d96d9 Compare December 21, 2016 19:25

brtsai added 3 commits February 3, 2017 07:01

partial support for -p flags code refactored

3f95027

Merge branch 'master' into partial-p-flags

d5847ef

updating xcbuild to match fb version

fa71541

added partial support for -p flags in lsbom #207

Are you sure you want to change the base?

added partial support for -p flags in lsbom #207

Uh oh!

Conversation

brtsai commented Dec 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brtsai commented Dec 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brtsai commented Jan 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grp commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brtsai commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brtsai commented Jan 29, 2017 •

edited

Loading

grp commented Jan 31, 2017 •

edited

Loading

brtsai commented Feb 6, 2017 •

edited

Loading