Skip to content
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

morestl: add examples for std::optional, any, variant #79

Merged
merged 10 commits into from Jan 27, 2022

Conversation

theanalyst
Copy link
Contributor

Draft WIP #69 PR ...don't have a full tex system currently so will be (ab)using the CI for a bit while I still make up other examples...

@theanalyst theanalyst force-pushed the optional-ex branch 2 times, most recently from 004cfc9 to 879cfb7 Compare December 16, 2021 08:02
talk/morelanguage/morestl.tex Outdated Show resolved Hide resolved
talk/morelanguage/morestl.tex Outdated Show resolved Hide resolved
talk/morelanguage/morestl.tex Outdated Show resolved Hide resolved
@theanalyst theanalyst force-pushed the optional-ex branch 4 times, most recently from a06b21b to a407cdb Compare December 16, 2021 17:29
Signed-off-by: Abhishek Lekshmanan <abhishek.lekshmanan@cern.ch>
talk/morelanguage/morestl.tex Outdated Show resolved Hide resolved
talk/morelanguage/morestl.tex Outdated Show resolved Hide resolved
talk/morelanguage/morestl.tex Outdated Show resolved Hide resolved
Signed-off-by: Abhishek Lekshmanan <abhishek.lekshmanan@cern.ch>
Signed-off-by: Abhishek Lekshmanan <abhishek.lekshmanan@cern.ch>
Signed-off-by: Abhishek Lekshmanan <abhishek.lekshmanan@cern.ch>
Signed-off-by: Abhishek Lekshmanan <abhishek.lekshmanan@cern.ch>
Signed-off-by: Abhishek Lekshmanan <abhishek.lekshmanan@cern.ch>
@theanalyst theanalyst force-pushed the optional-ex branch 2 times, most recently from a02f907 to d3f3914 Compare December 17, 2021 10:25
Signed-off-by: Abhishek Lekshmanan <abhishek.lekshmanan@cern.ch>
@theanalyst theanalyst changed the title morestl: add example slide for std::optional morestl: add examples for std::optional, any, variant Dec 17, 2021
@theanalyst theanalyst marked this pull request as ready for review December 17, 2021 10:44
@theanalyst
Copy link
Contributor Author

Added slides on std::variant and std::any now; also a brief one on std::string_view. I think this is ready for review now, happy to change anything.

@sponce
Copy link
Contributor

sponce commented Dec 17, 2021

Now that the different slides are written, I feel like the original one should be dropped and the content distributed into the different sections. It feels odd to start with it, then switch to string_view and comes back to the 3 items, one by one.

talk/morelanguage/morestl.tex Outdated Show resolved Hide resolved
talk/morelanguage/morestl.tex Outdated Show resolved Hide resolved
talk/morelanguage/morestl.tex Outdated Show resolved Hide resolved
talk/morelanguage/morestl.tex Show resolved Hide resolved
@theanalyst
Copy link
Contributor Author

Now that the different slides are written, I feel like the original one should be dropped and the content distributed into the different sections. It feels odd to start with it, then switch to string_view and comes back to the 3 items, one by one.

This makes sense, also makes it easy slide estate planning wise so I could mostly go for one slide explaining the item and the next one with the program example

Co-authored-by: Stephan Hageboeck <stephan.hageboeck@cern.ch>
Co-authored-by: Stephan Hageboeck <stephan.hageboeck@cern.ch>
@sponce
Copy link
Contributor

sponce commented Jan 19, 2022

@theanalyst This is almost ready to be merged, but if you do not mind, I would go for dropping the introduction slide and merge it in the others. I can do it myself if you agree

Copy link
Contributor

@hageboeck hageboeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one formatting thing and one optional line break. Otherwise, the output of the program would look a bit complicated.

talk/morelanguage/morestl.tex Show resolved Hide resolved
talk/morelanguage/morestl.tex Show resolved Hide resolved
@theanalyst
Copy link
Contributor Author

@theanalyst This is almost ready to be merged, but if you do not mind, I would go for dropping the introduction slide and merge it in the others. I can do it myself if you agree

Yes sure go ahead, sorry things got a bit too busy after the vacation

@sponce
Copy link
Contributor

sponce commented Jan 26, 2022

Ok, will do. I was also busy until now, so starting only today. One question I have though. The code presented for std::any looks wrong to me :

 if (Monster *m = std::any_cast<Monster>(&e)) {
          m->take_hit(this->damage);
        } else if (Orc *o = std::any_cast<Orc>(&e)) {
          o->take_hit(this->damage*0.5);
        } else if (Cat *c = std::any_cast<Cat>(&e)) {//No!

it assumes std::any_cast will return nullptr, but it will throw std::bad_any_cast, not return

@hageboeck
Copy link
Contributor

it assumes std::any_cast will return nullptr, but it will throw std::bad_any_cast, not return

Actually, only the reference and universal reference versions will throw @sponce. The pointer versions do return nullptr. cppreference.
But what I find very confusing on first look is that the template argument is the type <Orc> whereas the argument of the cast is a pointer (&e).
This is different from what you normally have, e.g. static_cast<Type*>(&e). This is what we should discuss in detail. If we both get confused on first look (in slightly different directions, admittedly), learners might be confused as well.

@sponce
Copy link
Contributor

sponce commented Jan 26, 2022

On now I've seen the little "(1-3)" in the cpp reference ! Thanks @hageboeck for being more awake than I was.
I find this extremely confusing...

I anyway would not take an example using pointers. After all we want to teach people to drop them everywhere.

@sponce
Copy link
Contributor

sponce commented Jan 26, 2022

So I've created a new version of this one in #89 (could not push to @theanalyst' s branch). I've changed the examples for any and if constexpr. @hageboeck if you could review, would be appreciated

@sponce sponce merged commit 8439ed9 into hsf-training:master Jan 27, 2022
@theanalyst
Copy link
Contributor Author

it assumes std::any_cast will return nullptr, but it will throw std::bad_any_cast, not return

Actually, only the reference and universal reference versions will throw @sponce. The pointer versions do return nullptr. cppreference. But what I find very confusing on first look is that the template argument is the type <Orc> whereas the argument of the cast is a pointer (&e). This is different from what you normally have, e.g. static_cast<Type*>(&e). This is what we should discuss in detail. If we both get confused on first look (in slightly different directions, admittedly), learners might be confused as well.

Yes indeed, that was a mistake, should've been Type*<&e> good catch. Also agree teaching value/references are easier, lesser chances to footgun ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants