managarm / frigg

Lightweight C++ utilities and algorithms for system programming
MIT License
56 stars 20 forks source link

Bunch of minor usability improvements here and there #15

Closed piotrrak closed 2 years ago

piotrrak commented 2 years ago

Please let me know if you want any of those.

qookei commented 2 years ago

These all seem good to me. Does managarm compile with these changes applied?

piotrrak commented 2 years ago

Yea, 0001 branches have no dependencies.

ArsenArsen commented 2 years ago

does this align with previous commit message discipline?

same for the other prs

Geertiebear commented 2 years ago

Also make sure mlibc compiles with these changes.

avdgrinten commented 2 years ago

@ArsenArsen Do you mean the branch name? That is fine IMHO.

Regarding the changes: they all look fine to me. For quantifying the optional::value() function: when does this make a difference? This only really affects code that directly passes .value() into a function taking a rvalue reference, right? (Since std::move(foo.value()) already gave a rvalue reference before.)

ArsenArsen commented 2 years ago

no, I mean the commit message, elsewhere it's component: description, also [[nodiscard]] for lock objects isn't very descriptive

piotrrak commented 2 years ago

Regarding the changes: they all look fine to me. For quantifying the optional::value() function: when does this make a difference? This only really affects code that directly passes .value() into a function taking a rvalue reference, right? (Since std::move(foo.value()) already gave a rvalue reference before.)

One common case would be:

optional<T> foo();

T t1 = foo().value();
// You sure could without it:
T t2 = std::move(foo().value()); 
avdgrinten commented 2 years ago

Makes sense. LGTM.

piotrrak commented 2 years ago

Will resubmit split per modules with more explanatory and consistent commit messages tonight/tomorrow.

piotrrak commented 2 years ago

Also make sure mlibc compiles with these changes.

mlibc also builds with those changes