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

C++ modernization #6

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@voldyman
Contributor

voldyman commented Jul 24, 2016

started using more C++ features like defining function types, avoiding pointers, using anonymous namespaces instead of static functions.

@voldyman voldyman changed the title from started using more c++14 features to C++ mordernization and print similar files as soon as it's found Jul 24, 2016

@voldyman voldyman changed the title from C++ mordernization and print similar files as soon as it's found to C++ mordernization Jul 25, 2016

@voldyman

This comment has been minimized.

Show comment
Hide comment
@voldyman

voldyman Jul 25, 2016

Contributor

updated the pull request to remove the commit that did the streaming print of result.

Contributor

voldyman commented Jul 25, 2016

updated the pull request to remove the commit that did the streaming print of result.

@akojo

This comment has been minimized.

Show comment
Hide comment
@akojo

akojo Jul 25, 2016

Owner

A couple of stylistic points first:

  • I like the using namespace std; -idiom
  • I'm not very fond of hiding types behind using-declarations (or typedefs for that matter)
  • Unnamed namespaces do look nicer than static functions, I'll have to make a note to use them more in my code

I wrote walk() the way it is because I didn't want to create and destroy a new temporary string object for each file processed. So I just keep reusing the same memory area. Well, that and it's a nice trick, so I like to keep it around 😄.

BTW, strictly speaking walk() is no more noexcept after your changes since string concatenation may throw an exception (not that it matters in this case).

Owner

akojo commented Jul 25, 2016

A couple of stylistic points first:

  • I like the using namespace std; -idiom
  • I'm not very fond of hiding types behind using-declarations (or typedefs for that matter)
  • Unnamed namespaces do look nicer than static functions, I'll have to make a note to use them more in my code

I wrote walk() the way it is because I didn't want to create and destroy a new temporary string object for each file processed. So I just keep reusing the same memory area. Well, that and it's a nice trick, so I like to keep it around 😄.

BTW, strictly speaking walk() is no more noexcept after your changes since string concatenation may throw an exception (not that it matters in this case).

@akojo

This comment has been minimized.

Show comment
Hide comment
@akojo

akojo Jul 25, 2016

Owner

Oh, there's a typo in the commit message. Should be "modernisation"

Owner

akojo commented Jul 25, 2016

Oh, there's a typo in the commit message. Should be "modernisation"

@voldyman voldyman changed the title from C++ mordernization to C++ modernization Jul 25, 2016

@voldyman

This comment has been minimized.

Show comment
Hide comment
@voldyman

voldyman Jul 25, 2016

Contributor

I like the using namespace std; -idiom
oh, i didn't know that, i changed it because using namespace std pollutes the global namespace and introduces the possibility of name conflicts.

using std::string just make it clear which types are being imported from where, like you could later switch to a different implementation of string if need be without much hassle.

I understood that you had made the clever choice of updating the string in place but i find that right now the code is concise and clear but later on there might be a possibility where one would accidentally access the wrong memory address and cause a segfault (in the best situation)
since the strings are small, creating a temporary objection shouldn't have much performance impact, but i could implement the inplace thing using iterators if you want.

Contributor

voldyman commented Jul 25, 2016

I like the using namespace std; -idiom
oh, i didn't know that, i changed it because using namespace std pollutes the global namespace and introduces the possibility of name conflicts.

using std::string just make it clear which types are being imported from where, like you could later switch to a different implementation of string if need be without much hassle.

I understood that you had made the clever choice of updating the string in place but i find that right now the code is concise and clear but later on there might be a possibility where one would accidentally access the wrong memory address and cause a segfault (in the best situation)
since the strings are small, creating a temporary objection shouldn't have much performance impact, but i could implement the inplace thing using iterators if you want.

@akojo

This comment has been minimized.

Show comment
Hide comment
@akojo

akojo Jul 25, 2016

Owner

Well, these are pretty much issues of style, and in this case I like to stick with mine 😄

Owner

akojo commented Jul 25, 2016

Well, these are pretty much issues of style, and in this case I like to stick with mine 😄

@voldyman

This comment has been minimized.

Show comment
Hide comment
@voldyman

voldyman Jul 25, 2016

Contributor

updated the code to

  • added back 'using namespace std'
  • removed using *
  • made the path replacement inplace, no new string objects.
Contributor

voldyman commented Jul 25, 2016

updated the code to

  • added back 'using namespace std'
  • removed using *
  • made the path replacement inplace, no new string objects.

@akojo akojo closed this Jul 27, 2016

@voldyman

This comment has been minimized.

Show comment
Hide comment
@voldyman

voldyman Jul 28, 2016

Contributor

can i change something to get this merged? now there are no extra string object creations. the harmful strcpy has been replaced with safe inplace iterator inserts.

Contributor

voldyman commented Jul 28, 2016

can i change something to get this merged? now there are no extra string object creations. the harmful strcpy has been replaced with safe inplace iterator inserts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment