//base: Make ScopedTempDir::path() a GetPath() with a DCHECK

//base: Make ScopedTempDir::path() a GetPath() with a DCHECK

This CL changes the inline ScopedTempDir::path() to an out-of-line
GetPath() and enhances it with a DCHECK against the directory not
having been created yet.

FAQ

Q: Why GetPath?
A: Because of the added DCHECK in the path accessor, which needs
   #include "base/logging.h", there were two alternatives:
    (A) Keep the inline path() and #include "base/logging.h" in the header.
    (B) Change the inline path() to an out-of-line GetPath and implement it
        in the .cc file.

   (A) would affect compile-time, (B) affects run-time of the tests. I made no
   measuring, but obtaining the path is usually a one-off step in a test, so an
   unlikely hotspot. Therefore the trade-off seems much better for (B).

Q: Why deprecating path() instead of deleting it?
A: Due to the big amount of callsites (almost 600 files in the whole codebase),
   this change is rolled out in phases. This CL changes //base. After all of the
   codebase is converted, the deprecated path() will be deleted.

R=danakj@chromium.org
BUG=640599
Committed: https://crrev.com/411f4fc60df3a256dd2d15ab39c540228ed66e7f
Cr-Commit-Position: refs/heads/master@{#417241}

[vabr (Chromium), 2016-08-24 12:57:17]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 12:57:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-08-24 13:02:11]

vabr@chromium.org changed reviewers:
+ danakj@chromium.org

[vabr (Chromium), 2016-08-24 13:02:12]

Hello danakj@!

Could you please review this?

My goal is to provide a warning against landing patches which will dump test
files into the current directory.

Currently I propose to do this via a DCHECK in path(). DCHECK is said [1] to
mean "this condition must always be true", which seems to fit this case: whoever
writes the test should make sure path() is only ever queried after creating the
directory.

Looking forward to your comments!
Vaclav


[1]
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[commit-bot: I haz the power, 2016-08-24 13:27:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 13:27:16]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[vabr (Chromium), 2016-08-24 13:33:36]

On 2016/08/24 13:27:16, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

The failing tests could actually be candidates for fixing, which consider doing
once I get agreement on using path() when the dir is invalid being a bad thing.

Cheers,
Vaclav

[danakj, 2016-08-25 23:47:52]

On 2016/08/24 13:33:36, vabr (OOO until 29 Aug) wrote:
> On 2016/08/24 13:27:16, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
> 
> The failing tests could actually be candidates for fixing, which consider
doing
> once I get agreement on using path() when the dir is invalid being a bad
thing.

[ RUN      ] FileProxyTest.CreateOrOpen_AbandonedCreate
[5192:5192:0824/063928:24069372517:FATAL:thread_restrictions.cc(38)] Check
failed: false. Function marked as IO-only was called from a thread that
disallows IO!  If this thread really should be allowed to make IO calls, adjust
the call to base::ThreadRestrictions::SetIOAllowed() in this thread's startup.
#0 0x0000005369f1 __interceptor_backtrace
#1 0x00000158b453 base::debug::StackTrace::StackTrace()
#2 0x0000015ef1ee logging::LogMessage::~LogMessage()
#3 0x00000171dab1 base::ThreadRestrictions::AssertIOAllowed()
#4 0x0000015c75b6 base::DirectoryExists()
#5 0x0000015d9802 base::ScopedTempDir::IsValid()
#6 0x0000007d1157
base::FileProxyTest_CreateOrOpen_AbandonedCreate_Test::TestBody()

IsValid() can't be used in all the places that we grab the path(). But why do we
need to check DirectoryExists() to know if it's valid. The class knows if it
made the dir or not. Is it trying to guard against 3rd parties deleting it
after?

[danakj, 2016-08-25 23:48:43]

On 2016/08/25 23:47:52, danakj wrote:
> On 2016/08/24 13:33:36, vabr (OOO until 29 Aug) wrote:
> > On 2016/08/24 13:27:16, commit-bot: I haz the power wrote:
> > > Dry run: Try jobs failed on following builders:
> > >   cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
> > 
> > The failing tests could actually be candidates for fixing, which consider
> doing
> > once I get agreement on using path() when the dir is invalid being a bad
> thing.
> 
> [ RUN      ] FileProxyTest.CreateOrOpen_AbandonedCreate
> [5192:5192:0824/063928:24069372517:FATAL:thread_restrictions.cc(38)] Check
> failed: false. Function marked as IO-only was called from a thread that
> disallows IO!  If this thread really should be allowed to make IO calls,
adjust
> the call to base::ThreadRestrictions::SetIOAllowed() in this thread's startup.
> #0 0x0000005369f1 __interceptor_backtrace
> #1 0x00000158b453 base::debug::StackTrace::StackTrace()
> #2 0x0000015ef1ee logging::LogMessage::~LogMessage()
> #3 0x00000171dab1 base::ThreadRestrictions::AssertIOAllowed()
> #4 0x0000015c75b6 base::DirectoryExists()
> #5 0x0000015d9802 base::ScopedTempDir::IsValid()
> #6 0x0000007d1157
> base::FileProxyTest_CreateOrOpen_AbandonedCreate_Test::TestBody()
> 
> IsValid() can't be used in all the places that we grab the path(). But why do
we
> need to check DirectoryExists() to know if it's valid. The class knows if it
> made the dir or not. Is it trying to guard against 3rd parties deleting it
> after?

2ndly, this requires base/logging.h in the header. It'd be nice if that could be
in the .cc file. Tho I realize there are a lot of callers to path() so making it
not inline and naming it GetPath() or something would be a large change for you.

[vabr (Chromium), 2016-09-06 13:33:41]

Patchset #2 (id:20001) has been deleted

[vabr (Chromium), 2016-09-06 19:27:20]

Thanks for the good comments!

I do agree that unlike IsValid(), what I want to DCHECK is !path().empty(),
which essentially means "Create*" was called.

Also, path -> GetPath sounds very reasonable -- the improving effect on
compilation time is much stronger than any run-time performance degradation from
losing the inlining.

I started converting the callsites locally, but as a single CL this would be
unmanageable. So I'll create a series, with the first CL converting base/,
adding GetPath() as an alternative to path() and marking the latter as
deprecated. I'll keep this change in this CL and ping you for review when its
ready. The other CLs will convert chunks of the tree and I'll send them to
relevant OWNERS. The last CL will be for you again, removing path() for good.

Cheers,
Vaclav

[danakj, 2016-09-06 20:28:16]

On Tue, Sep 6, 2016 at 12:27 PM, <vabr@chromium.org> wrote:

> Thanks for the good comments!
>
> I do agree that unlike IsValid(), what I want to DCHECK is !path().empty(),
> which essentially means "Create*" was called.
>
> Also, path -> GetPath sounds very reasonable -- the improving effect on
> compilation time is much stronger than any run-time performance
> degradation from
> losing the inlining.
>
> I started converting the callsites locally, but as a single CL this would
> be
> unmanageable. So I'll create a series, with the first CL converting base/,
> adding GetPath() as an alternative to path() and marking the latter as
> deprecated. I'll keep this change in this CL and ping you for review when
> its
> ready. The other CLs will convert chunks of the tree and I'll send them to
> relevant OWNERS. The last CL will be for you again, removing path() for
> good.
>

Sounds good let me know when it's ready to look at.


>
> Cheers,
> Vaclav
>
> https://codereview.chromium.org/2275553005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vabr (Chromium), 2016-09-07 08:18:20]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 08:18:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-09-07 08:39:31]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 08:39:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-09-07 08:44:47]

Description was changed from

==========
Warn in ScopedTempDir::path() if not valid

This CL adds a warning against calling ScopedTempDir::path() if the directory
is not valid.

BUG=640599
==========

to

==========
//base: Warn in ScopedTempDir::path() if empty

This CL adds a warning against calling ScopedTempDir::path() if the directory
has not been created yet.

Because it adds a DCHECK in the path accessor, which needs
#include "base/logging.h", there were two alternatives:
 (A) Keep the inlined path() and #include "base/logging.h" in the header.
 (B) Change the inlined path() to an out-of-line GetPath and implement it
     in the .cc file.

(A) would affect compile-time, (B) affects run-time of the tests. I made no
measuring, but obtaining the path is usually a one-off step in a test, so an
unlikely hotspot. Therefore the trade-off seems much better for (B).

Due to the big amount of callsites (almost 600 files in the whole codebase),
this change is rolled out in phases. This CL changes //base. After all of the
codebase is converted, the deprecated path() will be deleted.

R=danakj@chromium.org
BUG=640599
==========

[vabr (Chromium), 2016-09-07 08:48:59]

Description was changed from

==========
//base: Warn in ScopedTempDir::path() if empty

This CL adds a warning against calling ScopedTempDir::path() if the directory
has not been created yet.

Because it adds a DCHECK in the path accessor, which needs
#include "base/logging.h", there were two alternatives:
 (A) Keep the inlined path() and #include "base/logging.h" in the header.
 (B) Change the inlined path() to an out-of-line GetPath and implement it
     in the .cc file.

(A) would affect compile-time, (B) affects run-time of the tests. I made no
measuring, but obtaining the path is usually a one-off step in a test, so an
unlikely hotspot. Therefore the trade-off seems much better for (B).

Due to the big amount of callsites (almost 600 files in the whole codebase),
this change is rolled out in phases. This CL changes //base. After all of the
codebase is converted, the deprecated path() will be deleted.

R=danakj@chromium.org
BUG=640599
==========

to

==========
//base: Make ScopedTempDir::path() a GetPath() with a DCHECK

This CL changes the inline ScopedTempDir::path() to an out-of-line
GetPath() and enhances it with a DCHECK against the directory not
having been created yet.

FAQ

Q: Why GetPath?
A: Because of the added DCHECK in the path accessor, which needs
   #include "base/logging.h", there were two alternatives:
    (A) Keep the inline path() and #include "base/logging.h" in the header.
    (B) Change the inline path() to an out-of-line GetPath and implement it
        in the .cc file.

   (A) would affect compile-time, (B) affects run-time of the tests. I made no
   measuring, but obtaining the path is usually a one-off step in a test, so an
   unlikely hotspot. Therefore the trade-off seems much better for (B).

Q: Why deprecating path() instead of deleting it?
A: Due to the big amount of callsites (almost 600 files in the whole codebase),
   this change is rolled out in phases. This CL changes //base. After all of the
   codebase is converted, the deprecated path() will be deleted.

R=danakj@chromium.org
BUG=640599
==========

[commit-bot: I haz the power, 2016-09-07 09:45:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 09:45:09]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-09-07 09:47:39]

Hi danakj@,

This CL now morphed into the first step: introducing GetPath and converting
//base to use it. I updated the summary and the trybots are happy. Please
review.

Thanks!
Vaclav

[vabr (Chromium), 2016-09-07 10:18:08]

Also, I plan to send a PSA to chromium-dev telling people to use GetPath() and
giving context for the curious. The plan is to send that after this CL lands, so
that GetPath is available already.

Cheers,
Vaclav

[danakj, 2016-09-07 20:47:18]

LGTM

https://codereview.chromium.org/2275553005/diff/50001/base/files/file_proxy_u...
File base/files/file_proxy_unittest.cc (right):

https://codereview.chromium.org/2275553005/diff/50001/base/files/file_proxy_u...
base/files/file_proxy_unittest.cc:91: const FilePath& test_dir_path() const {
return dir_.GetPath(); }
Can you name these not-hacker-case too?

https://codereview.chromium.org/2275553005/diff/50001/base/files/file_util_pr...
File base/files/file_util_proxy_unittest.cc (right):

https://codereview.chromium.org/2275553005/diff/50001/base/files/file_util_pr...
base/files/file_util_proxy_unittest.cc:45: const FilePath& test_dir_path() const
{ return dir_.GetPath(); }
dittos

[vabr (Chromium), 2016-09-08 07:44:30]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-09-08 07:44:30]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2275553005/#ps70001
(title: "Comments")

[vabr (Chromium), 2016-09-08 07:45:36]

Thanks!

I removed one test_dir_path() because it was not used (see below), and also ran
git cl format on the result. If you want me to do any follow-ups, please just
let me know.

Cheers,
Vaclav

https://codereview.chromium.org/2275553005/diff/50001/base/files/file_proxy_u...
File base/files/file_proxy_unittest.cc (right):

https://codereview.chromium.org/2275553005/diff/50001/base/files/file_proxy_u...
base/files/file_proxy_unittest.cc:91: const FilePath& test_dir_path() const {
return dir_.GetPath(); }
On 2016/09/07 20:47:18, danakj wrote:
> Can you name these not-hacker-case too?

Done.

https://codereview.chromium.org/2275553005/diff/50001/base/files/file_util_pr...
File base/files/file_util_proxy_unittest.cc (right):

https://codereview.chromium.org/2275553005/diff/50001/base/files/file_util_pr...
base/files/file_util_proxy_unittest.cc:45: const FilePath& test_dir_path() const
{ return dir_.GetPath(); }
On 2016/09/07 20:47:18, danakj wrote:
> dittos

test_path -- done
test_dir_path -- not used anywhere, so removed

[commit-bot: I haz the power, 2016-09-08 07:50:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 09:26:45]

Description was changed from

==========
//base: Make ScopedTempDir::path() a GetPath() with a DCHECK

This CL changes the inline ScopedTempDir::path() to an out-of-line
GetPath() and enhances it with a DCHECK against the directory not
having been created yet.

FAQ

Q: Why GetPath?
A: Because of the added DCHECK in the path accessor, which needs
   #include "base/logging.h", there were two alternatives:
    (A) Keep the inline path() and #include "base/logging.h" in the header.
    (B) Change the inline path() to an out-of-line GetPath and implement it
        in the .cc file.

   (A) would affect compile-time, (B) affects run-time of the tests. I made no
   measuring, but obtaining the path is usually a one-off step in a test, so an
   unlikely hotspot. Therefore the trade-off seems much better for (B).

Q: Why deprecating path() instead of deleting it?
A: Due to the big amount of callsites (almost 600 files in the whole codebase),
   this change is rolled out in phases. This CL changes //base. After all of the
   codebase is converted, the deprecated path() will be deleted.

R=danakj@chromium.org
BUG=640599
==========

to

==========
//base: Make ScopedTempDir::path() a GetPath() with a DCHECK

This CL changes the inline ScopedTempDir::path() to an out-of-line
GetPath() and enhances it with a DCHECK against the directory not
having been created yet.

FAQ

Q: Why GetPath?
A: Because of the added DCHECK in the path accessor, which needs
   #include "base/logging.h", there were two alternatives:
    (A) Keep the inline path() and #include "base/logging.h" in the header.
    (B) Change the inline path() to an out-of-line GetPath and implement it
        in the .cc file.

   (A) would affect compile-time, (B) affects run-time of the tests. I made no
   measuring, but obtaining the path is usually a one-off step in a test, so an
   unlikely hotspot. Therefore the trade-off seems much better for (B).

Q: Why deprecating path() instead of deleting it?
A: Due to the big amount of callsites (almost 600 files in the whole codebase),
   this change is rolled out in phases. This CL changes //base. After all of the
   codebase is converted, the deprecated path() will be deleted.

R=danakj@chromium.org
BUG=640599
==========

[commit-bot: I haz the power, 2016-09-08 09:26:46]

Committed patchset #4 (id:70001)

[commit-bot: I haz the power, 2016-09-08 09:28:36]

Description was changed from

==========
//base: Make ScopedTempDir::path() a GetPath() with a DCHECK

This CL changes the inline ScopedTempDir::path() to an out-of-line
GetPath() and enhances it with a DCHECK against the directory not
having been created yet.

FAQ

Q: Why GetPath?
A: Because of the added DCHECK in the path accessor, which needs
   #include "base/logging.h", there were two alternatives:
    (A) Keep the inline path() and #include "base/logging.h" in the header.
    (B) Change the inline path() to an out-of-line GetPath and implement it
        in the .cc file.

   (A) would affect compile-time, (B) affects run-time of the tests. I made no
   measuring, but obtaining the path is usually a one-off step in a test, so an
   unlikely hotspot. Therefore the trade-off seems much better for (B).

Q: Why deprecating path() instead of deleting it?
A: Due to the big amount of callsites (almost 600 files in the whole codebase),
   this change is rolled out in phases. This CL changes //base. After all of the
   codebase is converted, the deprecated path() will be deleted.

R=danakj@chromium.org
BUG=640599
==========

to

==========
//base: Make ScopedTempDir::path() a GetPath() with a DCHECK

This CL changes the inline ScopedTempDir::path() to an out-of-line
GetPath() and enhances it with a DCHECK against the directory not
having been created yet.

FAQ

Q: Why GetPath?
A: Because of the added DCHECK in the path accessor, which needs
   #include "base/logging.h", there were two alternatives:
    (A) Keep the inline path() and #include "base/logging.h" in the header.
    (B) Change the inline path() to an out-of-line GetPath and implement it
        in the .cc file.

   (A) would affect compile-time, (B) affects run-time of the tests. I made no
   measuring, but obtaining the path is usually a one-off step in a test, so an
   unlikely hotspot. Therefore the trade-off seems much better for (B).

Q: Why deprecating path() instead of deleting it?
A: Due to the big amount of callsites (almost 600 files in the whole codebase),
   this change is rolled out in phases. This CL changes //base. After all of the
   codebase is converted, the deprecated path() will be deleted.

R=danakj@chromium.org
BUG=640599

Committed: https://crrev.com/411f4fc60df3a256dd2d15ab39c540228ed66e7f
Cr-Commit-Position: refs/heads/master@{#417241}
==========

[commit-bot: I haz the power, 2016-09-08 09:28:37]

Patchset 4 (id:??) landed as
https://crrev.com/411f4fc60df3a256dd2d15ab39c540228ed66e7f
Cr-Commit-Position: refs/heads/master@{#417241}