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

Adding 150 characters to all the buffers #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cmaureir
Copy link
Collaborator

Using 50 is really low, and in modern times we do not
need to care much, then using 200 is totally fine.

Issue-number: 4

Signed-off-by: Cristian Maureira-Fredes [email protected]

Using 50 is really low, and in modern times we do not
need to care much, then using 200 is totally fine.

Issue-number: 4

Signed-off-by: Cristian Maureira-Fredes <[email protected]>
@teuben
Copy link
Contributor

teuben commented Mar 28, 2018

Changing 50 to 200 is NOT the best solution:

  1. at least use a #define for that lenght, and use that throughout
  2. closer examination of the code should tell you how to use malloc() or something like that to use the correct amount.

I have a repo copy in https://github.com/teuben/mcluster which uses the following technique:

//Open output files
char *PARfile, *NBODYfile, *SSEfile;
FILE *PAR, *NBODY, *SSE12;
PARfile = strcat(output,".input");
PAR = fopen(PARfile,"w");
NBODYfile = strcat(output,".fort.10");
NBODY = fopen(NBODYfile,"w");

look at the start of main.c

This type of code is repeated over and over again, and could easily be uplifted in some utiity code so this doesn't have to be retyped all over the code. There was another issue on code-restructuring, which might involve this as well.

I thought I had made a pull request from my version, but seems not. Needs to be done or discussed what do to.

@cmaureir
Copy link
Collaborator Author

Well, I want it to be minimalistic as possible, this piece of code can be optimised in many ways.
I have rewritten the whole code to separate functions, creating structures, and many others things, but I cannot open a pull request that changes everything, that is why I monkey-patched the small open issues, and I was trying to submit changes little by little.

I think C is no the best way of doing things, so I have also a set of patched that move the whole thing to C++, not using classes, because that will scare people, but just using all the perks of C++ standard library.

So yeah, sorry for just increase the size of the buffers, but big changes are usually slow to get merge :)

@cmaureir
Copy link
Collaborator Author

This is the branch that I currently have with the whole restructuration of the project https://github.com/cmaureir/mcluster/tree/improvement

  • I'm moving everything to headers and lib files
  • After that I will check the proper functionallity of all the versions
  • To start, I added to big structures to hold the information of each particle and all the options that one can specify from the command line, but a deep look is necessary to have the proper structures, since there are many variables that don't really need to be inside them.
  • When this is working, you can clearly see that most of the functions are repeating code, so there is a real need to improve that, since I'm confident the code can be shorter as it's now.
  • The last stage will be to integrate std library algorithms to avoid the large amount of loops tha algorithms that the code currently have.

I'm more than happy to hear any further concern regarding the code, or maybe another important aspect that I'm missing.

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.

2 participants