crashpad: Introduce a new SettingsReader type for read-only access [crashpad/crashpad : main]
Joshua Pawlicki (Gerrit)
Joshua Pawlicki voted and added 6 comments![]( "Open in Gerrit")
Votes added by Joshua Pawlicki
| Commit-Queue | +1 |
6 comments
Mark, PTAL at this alternative direction to https://chromium-review.googlesource.com/c/crashpad/crashpad/+/7017459, thank you!
Unit testing is TODO pending general buy-in.
virtual bool OpenAndReadSettings(Data* out_data);Note: no attempt at locking or recovery is made in the SettingsReader; Settings overrides this to keep both behaviors.
The definition of Settings::OpenAndReadSettings is unchanged (not deleted), so it won't show up in the diff.
virtual bool Initialize();We discussed the question of the value of Settings::Initialize over chat, the value of SettingsReader::Initialize may be an entirely separate question, but I thought the change was most clearly presented with it as an impure virtual.
explicit SettingsReader(const base::FilePath& path);It was unclear to me whether there's value in wrapping this (plus Initialize, probably) with a unique-ptr-returning static factory function either here or in CrashReportDatabase, or whether it's just better to let the caller (O4) do a stack allocation and remember to check the return value of Initialize.
Settings::GetLastUploadAttemptTime moved to SettingsReader::GetLastUploadAttemptTime; no diff in implementation.
Settings::ReadSettings moved to SettingsReader::ReadSettings; no diff in implementation.
Related details
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 10 comments![]( "Open in Gerrit")
explicit CrashReportDatabaseGeneric(const base::FilePath& path);Why is this changing?
virtual bool OpenAndReadSettings(Data* out_data) override;If you say `override`, you don’t need to also say `virtual`. I think that no Crashpad code says both together, because there’s a warning that barks at you if you do. Or at least there used to be. Even if there’s not now, be consistent.
class Settings : public SettingsReader {With the `virtual` in the mix, this class should now be `final`.
const base::FilePath file_path_;
InitializationState initialized_;Better for these to be private, and to provide protected accessors for use by the derived class.
// |handle| must be the result of OpenForReading() or
// OpenForReadingAndWriting().again, weird thing to talk about in the SettingsReader class, unless you’re also going to introduce the concept of it being overridden here.
// failure. This does not perform recovery.Recovery’s a writer concept. I don’t think this description even makes any sense here unless you introduce it.
class SettingsReader {I don’t see a way to get a SettingsReader from CrashReportDatabase, which isn’t great because the CrashReportDatabase implementation is what decides where the settings live within it.
I don’t think that means that you need a CrashReportDatabase object to get a Settings. It’s fine for there to be a separate static CrashReportDatabase::GetSettingsForDatabasePath factory. The point is that the caller should only know a database path, and shouldn’t fish around any deeper than that. A database implementation is what knows how to get Settings within.
explicit SettingsReader(const base::FilePath& path);It was unclear to me whether there's value in wrapping this (plus Initialize, probably) with a unique-ptr-returning static factory function either here or in CrashReportDatabase, or whether it's just better to let the caller (O4) do a stack allocation and remember to check the return value of Initialize.
It was unclear to me whether there's value in wrapping this (plus Initialize, probably) with a unique-ptr-returning static factory function either here or in CrashReportDatabase, or whether it's just better to let the caller (O4) do a stack allocation and remember to check the return value of Initialize.
If the above comment pushes things in the direction of the factory, that might help make the decision easier.
bool Settings::Initialize() {Why did the `path` get removed from here? Just because of the inheritance? That produces a half-initialized state. I’d rather not do that.
Let’s continue doing all initialization in `Initialize`, and keep everything protected by the InitializationState object.
ScopedLockedFileHandle handle = OpenForReading();
if (!handle.is_valid())
return false;
if (ReadSettings(handle.get(), out_data, true))
return true;Can this call through to the base implementation?
Maybe not, since this seems to want to distinguish between what failed before deciding to do recovery.
Worth an explanatory comment, I guess.
Related details
- Joshua Pawlicki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joshua Pawlicki (Gerrit)
Joshua Pawlicki added 3 comments![]( "Open in Gerrit")
explicit CrashReportDatabaseGeneric(const base::FilePath& path);Why is this changing?
The `_win` and `_mac` database implementations take the path in their constructors, not in Initialize. Standardizing the pattern seemed desirable.
IMO standardizing on the constructor pattern is preferable to standardizing on passing the path in Initialize, since this approach reduces mutability (i.e. base_dir_ can become const).
This is also a prerequisite to passing the settings file path to Settings::Settings, which in turn allows Settings's file_path_ member to become immutable. That's the topic of another comment thread; regardless of how that resolves I still claim having the const member makes the class easier to understand, because it's immediately clear to a reader that base_dir won't change between calls nor after initialization.
If we standardize on the `_generic` approach of taking the path in Initialize, then we should consider adding INITIALIZATION_STATE_DCHECK_VALID to DatabasePath in all of _win, _mac, _generic. While learning this code, it wasn't clear to me if there's an intentional reason we don't guard that with the same initialization state check, if the path isn't guaranteed to be correct until post-initialization.
explicit SettingsReader(const base::FilePath& path);Mark MentovaiIt was unclear to me whether there's value in wrapping this (plus Initialize, probably) with a unique-ptr-returning static factory function either here or in CrashReportDatabase, or whether it's just better to let the caller (O4) do a stack allocation and remember to check the return value of Initialize.
It was unclear to me whether there's value in wrapping this (plus Initialize, probably) with a unique-ptr-returning static factory function either here or in CrashReportDatabase, or whether it's just better to let the caller (O4) do a stack allocation and remember to check the return value of Initialize.
If the above comment pushes things in the direction of the factory, that might help make the decision easier.
Yes, the above comment is a complete answer to this one.
bool Settings::Initialize() {Why did the `path` get removed from here? Just because of the inheritance? That produces a half-initialized state. I’d rather not do that.
Let’s continue doing all initialization in `Initialize`, and keep everything protected by the InitializationState object.
As mentioned above, the main benefit to passing `path` in the constructor is that the member can become `const`, which simplifies reasoning about the possibility that the member is ever modified between calls (the reader can determine that with 1 line of code read instead of having to read the entire file).
And, I also wanted to get your take on the idea of eliminating Initialize from the base class altogether, since it doesn't do anything there and is not needed.
Related details
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 3 comments![]( "Open in Gerrit")
explicit CrashReportDatabaseGeneric(const base::FilePath& path);Joshua PawlickiWhy is this changing?
The `_win` and `_mac` database implementations take the path in their constructors, not in Initialize. Standardizing the pattern seemed desirable.
IMO standardizing on the constructor pattern is preferable to standardizing on passing the path in Initialize, since this approach reduces mutability (i.e. base_dir_ can become const).
This is also a prerequisite to passing the settings file path to Settings::Settings, which in turn allows Settings's file_path_ member to become immutable. That's the topic of another comment thread; regardless of how that resolves I still claim having the const member makes the class easier to understand, because it's immediately clear to a reader that base_dir won't change between calls nor after initialization.
If we standardize on the `_generic` approach of taking the path in Initialize, then we should consider adding INITIALIZATION_STATE_DCHECK_VALID to DatabasePath in all of _win, _mac, _generic. While learning this code, it wasn't clear to me if there's an intentional reason we don't guard that with the same initialization state check, if the path isn't guaranteed to be correct until post-initialization.
The `_win` and `_mac` database implementations take the path in their constructors, not in Initialize. Standardizing the pattern seemed desirable.
IMO standardizing on the constructor pattern is preferable to standardizing on passing the path in Initialize, since this approach reduces mutability (i.e. base_dir_ can become const).
OK, that’s good enough for me.
bool SettingsReader::OpenAndReadSettings(Data* out_data) {Call out somewhere that the base SettingsReader’s variant doesn’t participate in locking, while the derived Settings implementation does.
bool Settings::Initialize() {Joshua PawlickiWhy did the `path` get removed from here? Just because of the inheritance? That produces a half-initialized state. I’d rather not do that.
Let’s continue doing all initialization in `Initialize`, and keep everything protected by the InitializationState object.
As mentioned above, the main benefit to passing `path` in the constructor is that the member can become `const`, which simplifies reasoning about the possibility that the member is ever modified between calls (the reader can determine that with 1 line of code read instead of having to read the entire file).
And, I also wanted to get your take on the idea of eliminating Initialize from the base class altogether, since it doesn't do anything there and is not needed.
As mentioned above, the main benefit to passing `path` in the constructor is that the member can become `const`, which simplifies reasoning about the possibility that the member is ever modified between calls (the reader can determine that with 1 line of code read instead of having to read the entire file).
OK, assuming we go with that…
And, I also wanted to get your take on the idea of eliminating Initialize from the base class altogether, since it doesn't do anything there and is not needed.
It’s still important in the derived class, and needs to be checked by the public interfaces available from the derived class, some of which are implemented in the base.
So you can either leave it in the base (along with the quirky requirement that SettingsReader users call Initialize, even though there’s nothing to initialize), or you can wrap all of the public methods that the derived class inherits from the base with little shims that check the InitializationState before calling through to the base (which is a lot of silly boilerplate busywork).
Or you can come up with a trick where a SettingsReader that’s not a Settings somehow magically springs into existence, constructed, with a valid InitializationState, and no Initialize method at that level at all. Meanwhile, a Settings subclass has its InitializationState (living in the SettingsReader base) set to uninitialized upon construction until its (non-override) Initialize is called.
Related details
- Joshua Pawlicki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joshua Pawlicki (Gerrit)
Joshua Pawlicki added 11 comments![]( "Open in Gerrit")
Mark, PTAL at this alternative direction to https://chromium-review.googlesource.com/c/crashpad/crashpad/+/7017459, thank you!
Unit testing is TODO pending general buy-in.
Done
virtual bool OpenAndReadSettings(Data* out_data) override;If you say `override`, you don’t need to also say `virtual`. I think that no Crashpad code says both together, because there’s a warning that barks at you if you do. Or at least there used to be. Even if there’s not now, be consistent.
Done
With the `virtual` in the mix, this class should now be `final`.
Done
const base::FilePath file_path_;
InitializationState initialized_;Better for these to be private, and to provide protected accessors for use by the derived class.
Done, but teach me: why is that important?
(Is it only because someone outside the project might introduce another extensions of SettingsReader, and we want to maintain flexibility to change the implementation of the accessor without breaking their code?)
// |handle| must be the result of OpenForReading() or
// OpenForReadingAndWriting().again, weird thing to talk about in the SettingsReader class, unless you’re also going to introduce the concept of it being overridden here.
Done
Recovery’s a writer concept. I don’t think this description even makes any sense here unless you introduce it.
Done
I don’t see a way to get a SettingsReader from CrashReportDatabase, which isn’t great because the CrashReportDatabase implementation is what decides where the settings live within it.
I don’t think that means that you need a CrashReportDatabase object to get a Settings. It’s fine for there to be a separate static CrashReportDatabase::GetSettingsForDatabasePath factory. The point is that the caller should only know a database path, and shouldn’t fish around any deeper than that. A database implementation is what knows how to get Settings within.
Done
virtual bool Initialize();We discussed the question of the value of Settings::Initialize over chat, the value of SettingsReader::Initialize may be an entirely separate question, but I thought the change was most clearly presented with it as an impure virtual.
Done
bool SettingsReader::OpenAndReadSettings(Data* out_data) {Call out somewhere that the base SettingsReader’s variant doesn’t participate in locking, while the derived Settings implementation does.
Done
Joshua PawlickiWhy did the `path` get removed from here? Just because of the inheritance? That produces a half-initialized state. I’d rather not do that.
Let’s continue doing all initialization in `Initialize`, and keep everything protected by the InitializationState object.
Mark MentovaiAs mentioned above, the main benefit to passing `path` in the constructor is that the member can become `const`, which simplifies reasoning about the possibility that the member is ever modified between calls (the reader can determine that with 1 line of code read instead of having to read the entire file).
And, I also wanted to get your take on the idea of eliminating Initialize from the base class altogether, since it doesn't do anything there and is not needed.
As mentioned above, the main benefit to passing `path` in the constructor is that the member can become `const`, which simplifies reasoning about the possibility that the member is ever modified between calls (the reader can determine that with 1 line of code read instead of having to read the entire file).
OK, assuming we go with that…
And, I also wanted to get your take on the idea of eliminating Initialize from the base class altogether, since it doesn't do anything there and is not needed.
It’s still important in the derived class, and needs to be checked by the public interfaces available from the derived class, some of which are implemented in the base.
So you can either leave it in the base (along with the quirky requirement that SettingsReader users call Initialize, even though there’s nothing to initialize), or you can wrap all of the public methods that the derived class inherits from the base with little shims that check the InitializationState before calling through to the base (which is a lot of silly boilerplate busywork).
Or you can come up with a trick where a SettingsReader that’s not a Settings somehow magically springs into existence, constructed, with a valid InitializationState, and no Initialize method at that level at all. Meanwhile, a Settings subclass has its InitializationState (living in the SettingsReader base) set to uninitialized upon construction until its (non-override) Initialize is called.
I explored option #3 a little bit but doing template meta-programming seemed more boilerplate than option #2, and passing a "needs_initialization" bool to the constructor seemed worse than #1.
I think I do prefer #2 to #1, and implemented it in patchset 7. My theory that if there is a significant difference on behavior between Settings::Get* and SettingsReader::Get*, it's not inappropriate for Settings to encode that using an override. However, I recognize that the distinction between that point of view and the alternative interpretation that both classes check the initialization state but that "SettingsReader" is always initialized (or has to be initialized in a do-nothing method) is slight at best and I definitely don't want to get hung up on this point, so if you strongly prefer #1 we can do that.
ScopedLockedFileHandle handle = OpenForReading();
if (!handle.is_valid())
return false;
if (ReadSettings(handle.get(), out_data, true))
return true;Can this call through to the base implementation?
Maybe not, since this seems to want to distinguish between what failed before deciding to do recovery.
Worth an explanatory comment, I guess.
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 9 comments![]( "Open in Gerrit")
static std::unique_ptr<SettingsReader> GetSettingsForDatabasePath(Returns a `SettingsReader`, should be named accordingly: `GetSettingsReaderForDatabasePath`.
//! \param[in] path A path to the database (not the settings file). If theThis may not be obvious if you’re coming at it from the perspective of the previous revisions of this change, but it’ll be obvious as checked in.
// static
std::unique_ptr<SettingsReader> CrashReportDatabase::GetSettingsForDatabasePath(
const base::FilePath& path) {
return std::make_unique<SettingsReader>(path.Append(kSettings));
}Group with the other static factory functions defined on `CrashReportDatabase`. You may want to borrow the pattern of the _mac.cc and _win.cc files by moving those other functions down here, or you may want to move this up and make the _mac.cc and _win.cc files match.
protected:Fine, although `protected` is also pointless in a `final` class, so I wouldn’t have had a problem with you leaving this where it was while adding the `override`. It’s permissible to downgrade access to a member in a derived class.
const base::FilePath file_path_;
InitializationState initialized_;Joshua PawlickiBetter for these to be private, and to provide protected accessors for use by the derived class.
Done, but teach me: why is that important?
(Is it only because someone outside the project might introduce another extensions of SettingsReader, and we want to maintain flexibility to change the implementation of the accessor without breaking their code?)
Done, but teach me: why is that important?
(Is it only because someone outside the project might introduce another extensions of SettingsReader, and we want to maintain flexibility to change the implementation of the accessor without breaking their code?)
1. Because [the style guide says so](https://google.github.io/styleguide/cppguide.html#Access_Control).
That’s not the strongest education on its own, though, so I’ll expand:
2. Similar to your reasoning for wanting this member to be `const`: providing a `private` member variable and a `protected` accessor but no mutator provides good assurance that the member can’t be altered outside of the base class. There are fewer places to look to assure yourself that it’s not used in surprising ways.
bool Settings::Initialize() {Joshua PawlickiWhy did the `path` get removed from here? Just because of the inheritance? That produces a half-initialized state. I’d rather not do that.
Let’s continue doing all initialization in `Initialize`, and keep everything protected by the InitializationState object.
Mark MentovaiAs mentioned above, the main benefit to passing `path` in the constructor is that the member can become `const`, which simplifies reasoning about the possibility that the member is ever modified between calls (the reader can determine that with 1 line of code read instead of having to read the entire file).
And, I also wanted to get your take on the idea of eliminating Initialize from the base class altogether, since it doesn't do anything there and is not needed.
Joshua PawlickiAs mentioned above, the main benefit to passing `path` in the constructor is that the member can become `const`, which simplifies reasoning about the possibility that the member is ever modified between calls (the reader can determine that with 1 line of code read instead of having to read the entire file).
OK, assuming we go with that…
And, I also wanted to get your take on the idea of eliminating Initialize from the base class altogether, since it doesn't do anything there and is not needed.
It’s still important in the derived class, and needs to be checked by the public interfaces available from the derived class, some of which are implemented in the base.
So you can either leave it in the base (along with the quirky requirement that SettingsReader users call Initialize, even though there’s nothing to initialize), or you can wrap all of the public methods that the derived class inherits from the base with little shims that check the InitializationState before calling through to the base (which is a lot of silly boilerplate busywork).
Or you can come up with a trick where a SettingsReader that’s not a Settings somehow magically springs into existence, constructed, with a valid InitializationState, and no Initialize method at that level at all. Meanwhile, a Settings subclass has its InitializationState (living in the SettingsReader base) set to uninitialized upon construction until its (non-override) Initialize is called.
I explored option #3 a little bit but doing template meta-programming seemed more boilerplate than option #2, and passing a "needs_initialization" bool to the constructor seemed worse than #1.
I think I do prefer #2 to #1, and implemented it in patchset 7. My theory that if there is a significant difference on behavior between Settings::Get* and SettingsReader::Get*, it's not inappropriate for Settings to encode that using an override. However, I recognize that the distinction between that point of view and the alternative interpretation that both classes check the initialization state but that "SettingsReader" is always initialized (or has to be initialized in a do-nothing method) is slight at best and I definitely don't want to get hung up on this point, so if you strongly prefer #1 we can do that.
I explored option #3 a little bit but doing template meta-programming seemed more boilerplate than option #2, and passing a "needs_initialization" bool to the constructor seemed worse than #1.
Template meta-programming? Oh my!
Here’s what I had in mind:
```
class SettingsReader {
public:
explicit Settings(FilePath const& path)
: Settings(path, InitializationSate::kStateValid) {}
protected:
Settings(FilePath const& path, InitializationState::State state)
: path_(path), state_(state) {}
};
class Settings : public SettingsReader {
public:
explicit Settings(FilePath const&)
: SettingsReader(path, InitializationState::kStateUninitialized) {}
};
```As implemented in patch set 7, the additional boilerplate which would need to be implemented for any future extensions to this class is uncomfortably easy to miss. Patch set 7 is also more `virtual` bloat without strong justification.
EXPECT_TRUE(reader.GetUploadsEnabled(&enabled));I’d normally ask for `ASSERT_TRUE` but I see that this is done elsewhere in this test, and it’s always `EXPECT_TRUE`. I can also find other cases where `ASSERT_TRUE` should have been used, such as around `Initialize` calls.
No need to clean all of this up now. You can do this to match the (imperfect) surroundings.
EXPECT_TRUE(enabled); // Not modified.There’s no documented interface requirement to not touch or touch this, so there’s no reason to test it.
}You also need to test that settings modified by the derived `Settings` class are properly read by the base `SettingsReader`. You don’t have to test every property, but it would be good to test `GetUploadsEnabled` as above, since that’s what you’re interested in using.
Related details
- Joshua Pawlicki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
bool Settings::Initialize() {Related details
- Joshua Pawlicki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joshua Pawlicki (Gerrit)
Joshua Pawlicki added 7 comments![]( "Open in Gerrit")
static std::unique_ptr<SettingsReader> GetSettingsForDatabasePath(Returns a `SettingsReader`, should be named accordingly: `GetSettingsReaderForDatabasePath`.
Done
//! \param[in] path A path to the database (not the settings file). If theThis may not be obvious if you’re coming at it from the perspective of the previous revisions of this change, but it’ll be obvious as checked in.
std::unique_ptr<SettingsReader> CrashReportDatabase::GetSettingsForDatabasePath(
const base::FilePath& path) {
return std::make_unique<SettingsReader>(path.Append(kSettings));
}Group with the other static factory functions defined on `CrashReportDatabase`. You may want to borrow the pattern of the _mac.cc and _win.cc files by moving those other functions down here, or you may want to move this up and make the _mac.cc and _win.cc files match.
Fine, although `protected` is also pointless in a `final` class, so I wouldn’t have had a problem with you leaving this where it was while adding the `override`. It’s permissible to downgrade access to a member in a derived class.
Done
bool Settings::Initialize() {Ah, I didn't think of using a protected constructor, just the public one. Done.
There’s no documented interface requirement to not touch or touch this, so there’s no reason to test it.
Done
You also need to test that settings modified by the derived `Settings` class are properly read by the base `SettingsReader`. You don’t have to test every property, but it would be good to test `GetUploadsEnabled` as above, since that’s what you’re interested in using.
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 7 comments![]( "Open in Gerrit")
This is in very good shape now. Thanks, Joshua!
std::unique_ptr<CrashReportDatabase> InitializeInternal(So this is just `crashpad::InitializeInternal`? That’s not right.
You don’t have to deal with it here if you don’t want to, but can you fix it in a follow-up?
You also don’t have to do that if you don’t want to. In that case, just say so, and I’ll do it.
The easiest way to fix this would be to wrap it in an unnamed namespace.
Same in _win.cc.
std::unique_ptr<CrashReportDatabase> InitializeInternal((looking here because of the previous comment—the same commentary about when to fix and who should fix applies here too.)
This is in the middle of nowhere now, and seemingly should also move down.
Settings(const base::FilePath& file_path);`explicit`
virtual bool GetClientID(UUID* client_id);This isn’t overriden, so it no longer needs to be `virtual`. The same applies to the 2 other `Get*` methods.
//! This class must not be instantiated directly, but rather an instance of it
//! should be retrieved via CrashReportDatabase::GetSettings().We had this nice comment telling people not to create these objects directly on their own, but to get them from the database. We still have that for `Settings`, it’s just moved down a bit in the file.
We should have a similar comment for `SettingsReader`, referring to the new function you added in `CrashReportDatabase`.
DCHECK(initialized().is_valid());Leave a blank line after the `DCHECK`, the same way that the old code used to, in all 3 `Get*` methods. Separating the preconditions from the rest of the code helps break up the flow and makes it easier to read. It might also encourage a better diff and better future blame.
Related details
- Joshua Pawlicki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joshua Pawlicki (Gerrit)
Joshua Pawlicki added 6 comments![]( "Open in Gerrit")
std::unique_ptr<CrashReportDatabase> InitializeInternal(So this is just `crashpad::InitializeInternal`? That’s not right.
You don’t have to deal with it here if you don’t want to, but can you fix it in a follow-up?
You also don’t have to do that if you don’t want to. In that case, just say so, and I’ll do it.
The easiest way to fix this would be to wrap it in an unnamed namespace.
Same in _win.cc.
Done; I previously wondered if the entire CrashReportDatabaseMac could/should be moved into an anonymous namespace, which could possibly help a little bit with clarity and might reduce the chance of this kind of mistake in the future, but I didn't go there in this patch set.
std::unique_ptr<CrashReportDatabase> InitializeInternal((looking here because of the previous comment—the same commentary about when to fix and who should fix applies here too.)
This is in the middle of nowhere now, and seemingly should also move down.
Done
Settings(const base::FilePath& file_path);Joshua Pawlicki`explicit`
Done
This isn’t overriden, so it no longer needs to be `virtual`. The same applies to the 2 other `Get*` methods.
Good point, forgot to remove those. Done.
//! This class must not be instantiated directly, but rather an instance of it
//! should be retrieved via CrashReportDatabase::GetSettings().We had this nice comment telling people not to create these objects directly on their own, but to get them from the database. We still have that for `Settings`, it’s just moved down a bit in the file.
We should have a similar comment for `SettingsReader`, referring to the new function you added in `CrashReportDatabase`.
Yeah, I wondered if it would come up. I previously struggled a little bit with it, specifically the "must not". Obviously, the tests instantiate it directly, as does the derived class. So I had left it off.
I took a stab at a new comment, LMK if you think it's short of the mark.
Leave a blank line after the `DCHECK`, the same way that the old code used to, in all 3 `Get*` methods. Separating the preconditions from the rest of the code helps break up the flow and makes it easier to read. It might also encourage a better diff and better future blame.
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 2 comments![]( "Open in Gerrit")
std::unique_ptr<CrashReportDatabase> InitializeInternal(Joshua PawlickiSo this is just `crashpad::InitializeInternal`? That’s not right.
You don’t have to deal with it here if you don’t want to, but can you fix it in a follow-up?
You also don’t have to do that if you don’t want to. In that case, just say so, and I’ll do it.
The easiest way to fix this would be to wrap it in an unnamed namespace.
Same in _win.cc.
Done; I previously wondered if the entire CrashReportDatabaseMac could/should be moved into an anonymous namespace, which could possibly help a little bit with clarity and might reduce the chance of this kind of mistake in the future, but I didn't go there in this patch set.
Done; I previously wondered if the entire CrashReportDatabaseMac could/should be moved into an anonymous namespace, which could possibly help a little bit with clarity and might reduce the chance of this kind of mistake in the future, but I didn't go there in this patch set.
It could, and doing so would be very much in line with Crashpad style. Same for the `…Generic` and `…Win` implementations.
I think the `friend` declarations in the client/crash_report_database.h are the reason that these classes aren’t in unnamed namespaces.
//! Prefer to construct this instances of this class through
//! CrashReportDatabase::GetSettingsReaderForDatabasePath.Please strengthen this in line with the admonition in Settings.
This doesn’t provide guidance for a unit test. A unit test is implicitly permitted exceed the limits established by the documentation. The existing unit test for `Settings` already overcame the documented perimeter, instantiating and initializing its own `Settings` objects without touching `CrashReportDatabase`.
This particular instance of inheritance also involves a close coupling, and the inheritance an implementation detail, so in that regard, the comment doesn’t apply.
In general, read (and write!) these public interface comments as a users’ manual, not an implementers’ manual.
Related details
- Joshua Pawlicki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joshua Pawlicki (Gerrit)
Joshua Pawlicki added 1 comment![]( "Open in Gerrit")
//! Prefer to construct this instances of this class through
//! CrashReportDatabase::GetSettingsReaderForDatabasePath.Please strengthen this in line with the admonition in Settings.
This doesn’t provide guidance for a unit test. A unit test is implicitly permitted exceed the limits established by the documentation. The existing unit test for `Settings` already overcame the documented perimeter, instantiating and initializing its own `Settings` objects without touching `CrashReportDatabase`.
This particular instance of inheritance also involves a close coupling, and the inheritance an implementation detail, so in that regard, the comment doesn’t apply.
In general, read (and write!) these public interface comments as a users’ manual, not an implementers’ manual.
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
1 comment
Thanks for approaching this in a try-it-and-see, refine-as-you-go iterative process. I’m happy with where we wound up on this.
Related details
- Joshua Pawlicki
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joshua Pawlicki (Gerrit)
Joshua Pawlicki added 1 comment![]( "Open in Gerrit")
Thanks for approaching this in a try-it-and-see, refine-as-you-go iterative process. I’m happy with where we wound up on this.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joshua Pawlicki (Gerrit)
Joshua Pawlicki voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Crashpad LUCI CQ (Gerrit)
Crashpad LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
crashpad: Introduce a new SettingsReader type for read-only access
- M client/crash_report_database.h
- M client/crash_report_database_generic.cc
- M client/crash_report_database_mac.mm
- M client/crash_report_database_win.cc
- M client/settings.cc
- M client/settings.h
- M client/settings_test.cc
- M util/misc/initialization_state.h
Code-Review: +1 by Mark Mentovai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |