Fix UNSAFE_TODO in process_iterator_win.cc [chromium/src : main]
David Bienvenu (Gerrit)
David Bienvenu added 1 comment![]( "Open in Gerrit")
I'm not sure if EqualsCaseInsensitiveASCII is OK here. Gemini swears up and down that it is even though some non-ascii characters won't be handled correctly, unless the locale is set correctly.
Related details
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 1 comment![]( "Open in Gerrit")
base::span(reinterpret_cast<uint8_t*>(&entry_), sizeof(entry_)));this method should operate on the provided `entry` rather than `entry_`.
Related details
- David Bienvenu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bienvenu (Gerrit)
David Bienvenu added 1 comment![]( "Open in Gerrit")
base::span(reinterpret_cast<uint8_t*>(&entry_), sizeof(entry_)));this method should operate on the provided `entry` rather than `entry_`.
Done. It's tempting to just inline this code into CheckForNextProcess since it's the only caller of InitProcessEntry. wdyt?
Related details
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 1 comment![]( "Open in Gerrit")
InitProcessEntry(&entry_);how about:
```
entry_ = ProcessEntry{{.dwSize = sizeof(entry_)}};
```
let the language do the job for us rather than fiddle with span stuff.
Related details
- David Bienvenu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bienvenu (Gerrit)
David Bienvenu added 2 comments![]( "Open in Gerrit")
PTAL (and weigh in on the case insensitivity issue for non ascii characters). Thx!
how about:
```
entry_ = ProcessEntry{{.dwSize = sizeof(entry_)}};
```
let the language do the job for us rather than fiddle with span stuff.
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 1 comment![]( "Open in Gerrit")
return base::EqualsCaseInsensitiveASCII(executable_name_, entry_exe_view) &&`FilePath::CompareIgnoreCase` seems appropriate since `exe_file()` returns the basename of the file.
i'd be tempted to change `ProcessEntry::exe_file()` so that it returns a `const FilePath::CharType*` so that the type makes this more explicit.
if you want to stick with the ASCII comparison, we could change `NamedProcessIterator` so that it requires a US-ASCII `executable_name` arg. i think that's a more intrusive change, though, so i wouldn't really suggest that we actually go that way. as it is, it's not safe to assume that these are ASCII strings.
(also, not that we don't need `base::` in front of things here.)
Related details
- David Bienvenu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bienvenu (Gerrit)
David Bienvenu added 2 comments![]( "Open in Gerrit")
PTAL
return base::EqualsCaseInsensitiveASCII(executable_name_, entry_exe_view) &&`FilePath::CompareIgnoreCase` seems appropriate since `exe_file()` returns the basename of the file.
i'd be tempted to change `ProcessEntry::exe_file()` so that it returns a `const FilePath::CharType*` so that the type makes this more explicit.
if you want to stick with the ASCII comparison, we could change `NamedProcessIterator` so that it requires a US-ASCII `executable_name` arg. i think that's a more intrusive change, though, so i wouldn't really suggest that we actually go that way. as it is, it's not safe to assume that these are ASCII strings.
(also, not that we don't need `base::` in front of things here.)
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson voted and added 3 comments![]( "Open in Gerrit")
Votes added by Greg Thompson
| Code-Review | +1 |
3 comments
lgtm w/ two optional suggestions.
@fdo...@chromium.org: //base/OWNERS stamp, please.
std::wstring_view entry_exe_view(entry().exe_file());make this `FilePath::StringViewType` while you're at it? :-)
(no good deed goes unpunished...)
return FilePath::CompareIgnoreCase(executable_name_, entry_exe_view) == 0 &&nit: `FilePath::CompareEqualIgnoreCase(executable_name_, entry_exe_view)` rather than reimplement it here?
Related details
- David Bienvenu
- Francois Pierre Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bienvenu (Gerrit)
David Bienvenu added 2 comments![]( "Open in Gerrit")
make this `FilePath::StringViewType` while you're at it? :-)
(no good deed goes unpunished...)
Done
return FilePath::CompareIgnoreCase(executable_name_, entry_exe_view) == 0 &&nit: `FilePath::CompareEqualIgnoreCase(executable_name_, entry_exe_view)` rather than reimplement it here?
- Francois Pierre Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |