-
Notifications
You must be signed in to change notification settings - Fork 546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve drcachesim -indir and -outdir docs: directory contents are internal #6786
base: master
Are you sure you want to change the base?
Conversation
…ories If the user happens to, for example, create a file in this directory then drcachesim will complain that it doesn't understand the file. Warn the user that the contents of these directories are internal to the tool.
@@ -72,7 +72,8 @@ droption_t<std::string> op_ipc_name( | |||
droption_t<std::string> op_outdir( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve -indir,-outdir docs, directory contents are internal
grammar: a comma cannot connect separate sentences: replace with a colon or similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That it's technically bad grammer is left as a given.
That this level of grammar correctness is required is new ... noted for future reference. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comma is still there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which comma? The comma in "Improve drcachesim -indir and -outdir docs, directory contents are internal" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, see the original review request. Oh I see now that there was another comma which maybe caused confusion though asking for the colon should disambiguate: my original request is s/docs,/docs:/
.
Ping |
Normally you'd click on the re-request review to indicate it's ready for a look: otherwise the reviewer will assume it's still being worked on. |
|
@@ -1146,6 +1146,8 @@ $ bin64/drrun -t drcachesim -indir drmemtrace.app.pid.xxxx.dir/ | |||
|
|||
The direct results of the \p -offline run are raw, compacted files, stored | |||
in a \p raw/ subdirectory of the \p drmemtrace.app.pid.xxxx.dir directory. | |||
The contents of this directory are internal to the tool. Do not alter, add, | |||
or delete files here. | |||
The \p -indir option both converts the data to a canonical trace form and | |||
passes the resulting data to the cache simulator. The canonical trace data | |||
is stored by \p -indir in a \p trace/ subdirectory inside the \p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same warning applies to the trace/ subdir too. (Or clarify the above to explicitly state it's talking about the outer dir and not the raw/ subdir.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about say that the contents of drmemtrace.app.pid.xxxx.dir, and all its subdirectories, are internal to the tool ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
@@ -1147,6 +1147,8 @@ $ bin64/drrun -t drmemtrace -indir drmemtrace.app.pid.xxxx.dir/ | |||
|
|||
The direct results of the \p -offline run are raw, compacted files, stored | |||
in a \p raw/ subdirectory of the \p drmemtrace.app.pid.xxxx.dir directory. | |||
The contents of \p drmemtrace.app.pid.xxxx.dir, and all its subdirectories, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing "the": s/of/of the/
(be consistent with all the other references such as line 1149 and line 1154 which have "the")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect the sole addition of "the" here to be bad grammar.
The other instances you point out say "... of the drmemtrace.app.pid.xxxx.dir directory",
whereas here the wording is "... of drmemtrace.app.pid.xxxx.dir".
Shall I add "directory" as well ? [gotta get the grammar correct! :-)]
i.e., "The contents of the \p drmemtrace.app.pid.xxx.dir directory, and all its subdirectories, ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah agreed, must have read it as the same as the others: good as-is or with "the ... directory".
@@ -72,7 +72,8 @@ droption_t<std::string> op_ipc_name( | |||
droption_t<std::string> op_outdir( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the rebranding of the framework as "drmemtrace" (because "drcachesim" is just one tool inside the framework), best to s/drcachesim/drmemtrace/
in the PR title and description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except that these docs are specifically for drcachesim?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, these docs are not for the drcachesim simulator: they are for the drmemtrace framework's tracer and post-processor and front-end for running a variety of tools (just one of which is drcachesim).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. The option is defined in clients/drcachesim/common/options.cpp. Okey doke.
If the user happens to, for example, create a file in this directory then drcachesim will complain that it doesn't understand the file. Warn the user that the contents of these directories are internal to the tool.