Skip to content

Conversation

stewpend0us
Copy link
Contributor

@stewpend0us stewpend0us commented May 11, 2023

API should be the same but now, if you want to, you can specify the internal buffer size.
Included it in one of the examples.
Murdered the makefile...you don't have to keep that part...

fixes #29

… if they want to change them. also the default size is now a #define so that it can be globally changed at compile time
…ically detect changes in files and rebuild only if needed.......not that it matters for this project really.
@eminfedar
Copy link
Owner

Very nice PR! I have some findings:

  1. TCPServer and UDPServer don't have .Connect(), so with this PR, they can't set the BUFFER_SIZE. Servers should be able to set their buffer size too. (for example TCPServer creates TCPSockets and calls .Listen() on them, this .Listen<BUFFER_SIZE>() should be configurable like TCPSocket)
  2. I think setting the BUFFER_SIZE of a Socket will look better & reasonable in the constructor (when creating the object/socket):
TCPSocket<0xFFFF> socket;
TCPServer<0xFFFF> server;

UDPSocket<0xFFFF> udp_socket;
UDPServer<0xFFFF> udp_server;

Because a user might want to set the default buffer size for TCPSockets created in each incoming connection of a TCPServer? Same for other classes too.

  1. If we have an alternative to #define usage (like constexpr) and if we can make this with only constexprs I think it will be more modern C++. Probably it won't in this case but, #define constants can conflict & override each other if a user define something with the same name.

These are the points came to my mind. Feel free to share your opinions. Thank you for your contributions!

@stewpend0us
Copy link
Contributor Author

stewpend0us commented May 11, 2023

  1. Oops. Yeah missed that completely. I'll add it in.
  2. This is where I started but I couldn't figure out how to do it this way AND keep the temp buffers all local to their function and truly temp. You could create a temp buffer on the class and re-use it but I didn't like that as much. Could also make it actually dynamic and "new" an array but that seems like overkill. What do you think?
  3. I know constexpr is the new hotness but (as far as I know) you can't change their values from anywhere other than modifying the source? The nice thing about #define (with the #ifndef guard) is you can set the value at compile-time (with the -D flag I think). This one doesn't really bother me either way though because it'd be super easy for the user to make their own "default" value and just use it now...want me to change it back?

@stewpend0us
Copy link
Contributor Author

stewpend0us commented May 12, 2023

(two). Ok. I completely missed the easy way just making class templates like you suggested. Fixed.
(one). I think this will just work now?
(three). left this as #define for now. feel free to change it or let me know what you'd prefer.

Sorry i'm a bit new to PR's do I need to do anything since I made another commit? It seems like it just showed up here?

@eminfedar
Copy link
Owner

That is good! I'm merging it. Thank you for your efforts :)

Sorry i'm a bit new to PR's do I need to do anything since I made another commit? It seems like it just showed up here?

When you pushed your commits on your fork, it will show up here, then I can see them and merge them if they are ok, that's it :).

@eminfedar eminfedar merged commit d66588d into eminfedar:master May 12, 2023
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.

internal buffers are not configurable

2 participants