search

vegastrike / Vega-Strike-Engine-Source

Vega Strike Engine
Other
255 stars 44 forks source link

C11 - Remove compilation warnings #45

Closed nabaco closed 4 years ago

nabaco commented 4 years ago

Since moving to C11 standard in #35, there is a flood of compilation warnings, where are either of the following two:

It would be nice to remove the warnings and align our codebase with that standard.

nabaco commented 4 years ago

@royfalk Would you be willing to take this, as you seem to be more proficient with C11?

BenjamenMeyer commented 4 years ago

The std::auto_ptr just needs to be updated to a new smart pointer type; see https://en.wikipedia.org/wiki/Auto_ptr for some info; std::unique_ptr is the primary replacement, and std::shared_ptr might be useful too but we'll have to identify which ones need it. We can probably start by swapping them to std::unique_ptr. See also https://en.wikipedia.org/wiki/Smart_pointer#unique_ptr.

Not sure about the other one.

LifWirser commented 4 years ago

Congradulations somebody managed to make an issue out of something that was SOLVED.

Not only were the "auto_ptr" issue already dealt with, but the attempts to solve it AGAIN corrupted the 0.5.3 branch. NICE JOB

BenjamenMeyer commented 4 years ago

@LifWirser I'm not seeing any issue on the 0.5.3 branch. On master, this hasn't been solved. If there is a change that needs to be pushed to master to solve it then that should get pushed to master IMHO.

Also, we're seeing it on master now b/c of the addition of support for C++11 and C++11 deprecated std::auto_ptr.

nabaco commented 4 years ago

Currently both branches are corrupted due to a change that was merged, without CI checks or a PR review (df7f7d420dec684d0af775a8a0a1d6e9bb59bc0a). My PR should fix the issue, #29.

The issue with the auto_ptr appeared due to a decision to switch fully to C11. In my opinion, the decision was taken a bit to quickly, and without proper discussion, but I think I'm too new in the community to affect such decisions. Either way, if we don't want to revert the change, we need to adapt the code to it. It's your call.

BenjamenMeyer commented 4 years ago

@nabaco not sure a PR gate would have caught that without unit tests in place

royfalk commented 4 years ago

For the record, I’m the one who pushed the change. I actually debated whether to add this or not, but figured this would come up in the review. It was clearly marked as one of the changes in the PR.

I only merged the change after:

  1. It was approved (I think by Benjamin)
  2. It passed available CI builds (that were supposed to work)
  3. It worked on my machine (obviously)

My apologies for the distress this caused. We should of course revert it.

My vision for the code is a clean, modern one. I’m a new C++ programmer but an experienced one in general. I try to follow the guidelines of cppcon experts and they all push the new stuff.

With that, my suggestion and preferences (maybe go in an issue):

  1. C++20 > C++ 17 > C++14 > C++11 (assuming we can make it build and when 20 comes out)
  2. STL and other standard features over 3rd party (e.g. boost). E.g. unique pointer over auto_ptr.
  3. Readable easy code over faster code. For now, remove weird optimization code and trust the compiler to optimize, but verify performance didn’t drop noticeably. The code has places where there are comments of better programmers admitting defeat, after trying to refactor. Assuming we can, we should make it accessible for junior/low involvement people to contribute.
  4. Support: Newer environments and compilers. Older game machines. We’re here to have fun too. Supporting some old ugly code just so it can compile on Vax isn’t fun.

Got a bit long. I wanted to reply before but I’m swamped at my day job.

Regards, Roy

From: Benjamen Meyer notifications@github.com Reply-To: vegastrike/Vega-Strike-Engine-Source reply@reply.github.com Date: Tuesday, 7 April 2020 at 1:21 To: vegastrike/Vega-Strike-Engine-Source Vega-Strike-Engine-Source@noreply.github.com Cc: Roy Falk roy@falk.co.il, Mention mention@noreply.github.com Subject: Re: [vegastrike/Vega-Strike-Engine-Source] C11 - Remove compilation warnings (#45)

@nabacohttps://github.com/nabaco not sure a PR gate would have caught that without unit tests in place

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/vegastrike/Vega-Strike-Engine-Source/issues/45#issuecomment-610067476, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACQWBEEN2G65CO2AFQUEVN3RLJIWNANCNFSM4MA74JTQ.

nabaco commented 4 years ago

@nabaco not sure a PR gate would have caught that without unit tests in place

@BenjamenMeyer- compilation is broken. That's the only thing the CI would catch 😄

nabaco commented 4 years ago

@royfalk removing the C11 line in engine/CMakeLists.txt breaks compilation due to your changes in the particle system. Should we revert the entire commit?

royfalk commented 4 years ago

Are you sure it does? I only got a warning for C++11 without it. I’m beginning to think we should all standardize (at least for a while) around a specific environment. At least until we sort CI and decide on approved compilers.

From: Nachum Barcohen notifications@github.com Reply-To: vegastrike/Vega-Strike-Engine-Source reply@reply.github.com Date: Tuesday, 7 April 2020 at 10:08 To: vegastrike/Vega-Strike-Engine-Source Vega-Strike-Engine-Source@noreply.github.com Cc: Roy Falk roy@falk.co.il, Mention mention@noreply.github.com Subject: Re: [vegastrike/Vega-Strike-Engine-Source] C11 - Remove compilation warnings (#45)

@royfalkhttps://github.com/royfalk removing the C11 line in engine/CMakeLists.txt breaks compilation due to your changes in the particle system. Should we revert the entire commit?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/vegastrike/Vega-Strike-Engine-Source/issues/45#issuecomment-610215487, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACQWBEAKHWPY475YQQHQFDLRLLGNFANCNFSM4MA74JTQ.

BenjamenMeyer commented 4 years ago

In the CI builds (Ubuntu 14.04, 16.04, 18.04; and Mac OSX) it only generates warnings and things build. (16.04 has its own issues which I'm working to resolved, but they're around the isnan and isfinite functionality; separate issue.)

The move to C++11 might have been a little premature; but I think there's a lot there we can gain too and we have a system in place that can verify builds to a degree right now.

I do agree with @royfalk that we should standardize on a subset of compiles/platforms. I propose that we standardize on those we can easily do CI against and can verify is in working order - which basically leaves us with the Linux versions right now. I'd love to have OS X working, but that seems to be a little bit of effort to get Gtk+X11 swapped over to Gtk+Quartz and figure out some Py2/Py3 inter-dependencies on that platform. Windows is not well supported yet by Travis-CI; so without changing CI systems (CircleCI, etc) we're limited for the moment - that'll change.

As far as compilers, we seem to do best with GCC Suite right now too. So with all that said, are we good for standardizing on the Linux Platform with GCC for the time being?

For those on Windows-only, would a VM or WSL environment work for the time being?

NOTE: This doesn't mean other platforms can't be fixed, etc; just that we're only focused on ensuring one platform does work.

royfalk commented 4 years ago

I agree that we need confidence. We need to know our change didn’t break something for someone. Also agree we should focus on Linux and GCC. Next priorities should be Mac and clang. Regarding Windows, we should wait until we can implement it in CI.

From: Benjamen Meyer notifications@github.com Reply-To: vegastrike/Vega-Strike-Engine-Source reply@reply.github.com Date: Tuesday, 7 April 2020 at 16:56 To: vegastrike/Vega-Strike-Engine-Source Vega-Strike-Engine-Source@noreply.github.com Cc: Roy Falk roy@falk.co.il, Mention mention@noreply.github.com Subject: Re: [vegastrike/Vega-Strike-Engine-Source] C11 - Remove compilation warnings (#45)

In the CI builds (Ubuntu 14.04, 16.04, 18.04; and Mac OSX) it only generates warnings and things build. (16.04 has its own issues which I'm working to resolved, but they're around the isnan and isfinite functionality; separate issue.)

The move to C++11 might have been a little premature; but I think there's a lot there we can gain too and we have a system in place that can verify builds to a degree right now.

I do agree with @royfalkhttps://github.com/royfalk that we should standardize on a subset of compiles/platforms. I propose that we standardize on those we can easily do CI against and can verify is in working order - which basically leaves us with the Linux versions right now. I'd love to have OS X working, but that seems to be a little bit of effort to get Gtk+X11 swapped over to Gtk+Quartz and figure out some Py2/Py3 inter-dependencies on that platform. Windows is not well supported yet by Travis-CI; so without changing CI systems (CircleCI, etc) we're limited for the moment - that'll change.

As far as compilers, we seem to do best with GCC Suite right now too. So with all that said, are we good for standardizing on the Linux Platform with GCC for the time being?

For those on Windows-only, would a VM or WSL environment work for the time being?

NOTE: This doesn't mean other platforms can't be fixed, etc; just that we're only focused on ensuring one platform does work.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/vegastrike/Vega-Strike-Engine-Source/issues/45#issuecomment-610401459, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACQWBECB33RJNVNZTSKGRO3RLMWHHANCNFSM4MA74JTQ.

nabaco commented 4 years ago

For Windows CI we can try AppVoyer. For me developing on Windows WSL works well, but I can't manage to test anything, so I need to go to my Linux box for that. Clang compilation should be fixed and enabled in CI with #29, so I think we can include it too.

BenjamenMeyer commented 4 years ago

Let's get some confidence before tackling Windows in CI - one thing at a time. I'd also like to figure out how to get the tests working in CI too.

nabaco commented 4 years ago

Do we have any existing tests?

BenjamenMeyer commented 4 years ago

Let's move the Testing discussion to #52 Short answer: I don't think they do

stephengtuggy commented 4 years ago

@royfalk Good job taking responsibility for the mistake. We all make mistakes; it's a sign of a mature software developer that you were able to admit it. :clap:

I agree with just about everything else you said. Maybe just one point: Normally I would prefer STL smart pointer types over boost, but, not sure that STL has what we need. It looks to me like what we need is something called boost::intrusive_ptr, where each object instance keeps an internal counter of the references to it, then destroys itself after the counter reaches zero. This seems to fit best with the way the current code works. Further discussion over on issue #8 . I would appreciate your feedback there, as well.

Thanks!

For the record, I’m the one who pushed the change. I actually debated whether to add this or not, but figured this would come up in the review. It was clearly marked as one of the changes in the PR. I only merged the change after: 1. It was approved (I think by Benjamin) 2. It passed available CI builds (that were supposed to work) 3. It worked on my machine (obviously) My apologies for the distress this caused. We should of course revert it. My vision for the code is a clean, modern one. I’m a new C++ programmer but an experienced one in general. I try to follow the guidelines of cppcon experts and they all push the new stuff. With that, my suggestion and preferences (maybe go in an issue): 1. C++20 > C++ 17 > C++14 > C++11 (assuming we can make it build and when 20 comes out) 2. STL and other standard features over 3rd party (e.g. boost). E.g. unique pointer over auto_ptr. 3. Readable easy code over faster code. For now, remove weird optimization code and trust the compiler to optimize, but verify performance didn’t drop noticeably. The code has places where there are comments of better programmers admitting defeat, after trying to refactor. Assuming we can, we should make it accessible for junior/low involvement people to contribute. 4. Support: Newer environments and compilers. Older game machines. We’re here to have fun too. Supporting some old ugly code just so it can compile on Vax isn’t fun. Got a bit long. I wanted to reply before but I’m swamped at my day job. Regards, Roy

LifWirser commented 4 years ago

@BenjamenMeyer How is it then when I tried to clone 0.5.3 to verify a fix for current issues I started to get "auto_ptr compiler warenings when 0.5.3 had NONE before? Did you by chance sync 0.5.3 to mastrer insteade of the other way around (looks like it to me). Why do think I closed the smart pointer? I knew it was solved. I also knew the repository need to be redone , in a careful manner, to prevent what you appeared to have done to it. maybe if somebody payed attention to the RED flags github provides on the 0.5.3 tag you may see the problem. BTW 0.5.3 may not have a direct connection to master just as "taose" did not (at least in github terms).Tthe repository pre -BenjamenMeyer had branches gathered from multiple sources, and in fact needed to be reorganized _not wholesale regressed. If you cloned from the link provide by pryamid on the website forums you would have found the newest code Without "auto-ptr warning. In fact I was working on current C11 warnings when I found what happened to the 0.5.3 code. BTW did you have any testing performed to verify you Code did not cause further problems?

At this point I don't care if you take this personally or not, If you were wise you would try to learn . At this point I'll leave you to your appearant delusions of grandure

BenjamenMeyer commented 4 years ago

@LifWirser While I understand your frustations, please be aware that:

  1. The 0.5.3 Tag has not changed.
  2. I actually haven't touched the 0.5.3 branch; I've only worked on master (https://github.com/vegastrike/Vega-Strike-Engine-Source/pulls?q=is%3Apr+author%3ABenjamenMeyer+is%3Aclosed), and anything I have put up I have at least run vegastrike to verify to the degree I can.
  3. I do contribute reviews, but I've kept light on merging anything.
  4. The forums right now don't work well; I've been in there in the past when trying to get things going but do not have the history others here have there.
  5. If there is code out there that is not in GitHub that helps move us forward please bring it to GitHub.
  6. Most of what I'm doing is around project management - filing stories, building project boards etc. It'll be a while until I start doing a lot in the code itself. My primary goal is to (a) find people to work on this project - both new and old, and (b) get us all working together and communicating.
  7. I may propose a lot of things, but I'm not going to act on them until the community - those involved here - provides direction to do so. That may be like #56 where I'll get more involved, but primarily things like #34 and #41 .

Finally, it does not help to berate people. I understand the frustration perception of how things are going. There's a lot of learning going on by a lot of people. As someone more familiar with the code please be more active in code reviews and mentoring those of us that are not so familiar - we need your help there as we all get to know the projects and repos and build up our own knowledge of it all.

All code on 0.5.3 and master branches must pass the PR gate. Admins can override, but please don't right now. Let's get #34 figured out and start talking more in real-time - get to know each other, etc. I'll happily host a Zoom session with anyone wanting to meet up that way; it'd probably do us all a world of good too.

stephengtuggy commented 4 years ago

@LifWirser Whoa, whoa, dude, time out! No need to get like that!

Personally, I don't see any evidence that anyone is acting in bad faith here. Please, let's try to give each other a break. (And ourselves, too, when we need one!)

Pretty sure no code has been lost. At least not irretrievably. Even if 0.5.3 got rolled back, there are probably other copies of it lying around somewhere. And if not, then worse comes to worst, we can redo the changes. Right? At least I hope so.

@BenjamenMeyer I would be in favor of the Zoom session. But not today. Maybe tomorrow, or over the weekend. LOL. I think I'm needing a break for today. :sunglasses:

LifWirser commented 4 years ago

@stephengtuggy Having a more complete understanding of the situation ,I disagree

@BenjamenMeyer Idont recalling syncing 0.5.3 with the master, I apoligize if I accused you in error. However somebody did and in the process wiped out the latest version of the Vegastrike engine. @stephengtuggy do you have or know where to find any such copies?

Let me try to explain. When the gitHup vegastrike was created it was imported from the original Sourceforge Repository. Afterwhich two other reposituries were "grafted" as branches: taose which I had found to be supirior to the SF master and 0.5.3 which was different from both and taose and also supirior to the "master" . After this I was offered admin priviledges by prymid. I did not ask for them . They were offered to me based on my previous work on the project. While I am glad that other are willing to attempt to fill the various voids left by previous project members. I am concerned by the apparent lack of respect shown by those who lack the understanding (as demonstrated by working on the older "master"), the failure to heed warnings about the dangers of thier actions, or the refusal/inability to admit when mistakes are made. Just a bit of thisOG's background, I work on multiple command/control projects- Wher REAL LIVES were on the line. Working on systems where one single typo put people/property in real danger. Even if it only resulted in production loss the cost an engineer imformed me was about 7000 USD/minute. Only due to medical issues have I now have time to devote to this.

Some may hope to use Vegastrike to demonstrate your abilities in furthering carreer goals - then you had better learn how to listen. Lofty goals will often fail due to lack of proper planning. Take the "question about replacing Cmake, has anybody besides me looked at first removing remains from previous build systems? Or even where to find them?

While I have a passion for Vegastrike, I am at the point where I have to fade into the background and let those responsible clean up the mess((es) they made

BenjamenMeyer commented 4 years ago

@LifWirser Let's vid chat and we can work out the restoration. I'd like to understand more about the "wiped out the lastest version" side of things; however, based on previous experience I think this is going to be something we need to talk about face-to-face ala Zoom, etc. I have not seen anything merged through a PR where someone said "do not do this" or any similar language - in fact, we've had a PR closed (not merged) for exactly that reason.

Specifically, I want to know if it's possible to roll back to a previous git hash or not.

I don't see a branch in github named taose. Let' track that (see #61).

This isn't about making a name for myself, career goals, etc for me - it's about getting back a game I loved. Honestly even though I have a long history of C/C++ development, etc this has very little if anything to do with my career any more.

I only asked for permissions related to project management and have tried to respect that and keep my role generally to that, looking to others here for the rest of things.

I did compare the 0.5.3 branch with master, and I noticed there were commits being pushed outside of PRs. As you have complained about it being broken, I enabled the same protections on that branch as on master. Admins can still get around that, but let's do that only when absolutely necessary to fix something.

As I staid, I understand your frustration. My skin is thick enough to handle what was said - whether appropriately directed at me or not; let's just work this all out as a community. I'm more than happy - - even prefer - to differ to those such as yourself that have historic knowledge as we get folks up to snuff. One of the things we'll need to eventually decide on is how to control our core dev group as people come and go. So right now, think about who should be in that group and who is willing to step up to the requirements of that group too (since they'll have to be active to help keep the project moving) - please see #62 for that discussion.

ghost commented 4 years ago

Hello. I cloned that repo few days ago, had those warnings, fixed lot of those (but not all, there are some that will imply better understanding of the codebase, others that annoyed me too much) locally.

Just fetched the current state (763fbac3728223accc87a2fb0ace904735d55584 as of writing), rebased my changes on it (no conflicts, so I guess the lines I touched were not fixed since my original changes).

I'm attaching the patch here, it's pretty straigtforward, almost only one-line changes, except float implicit casts and the use of obsolete TR1. Feel free to use it, like it is or with changes.

I'll include also a patch to mention submodules in the README file. I don't think it's worth creating bureaucracy for that, if you do, well, do it (I dislike using github enough like this so forgive me for that... seriously, this stupid thing does not even accept .patch extension...).

Fixed_various_warnings.patch.tar.gz mentionning_submodules_in_README.md.patch.tar.gz

ghost commented 4 years ago

Oh, forgot to mention few things...

I use Debian 10 (buster) and clang 7. My CXX_FLAGS is usually: "-std=c++11 -Weverything -Wno-c++98-compat-pedantic -Wno-c++98-compat -stdlib=libc++" (and writing this I note that I should add some stuff there).

This patch has been built against both g++ in normal, default setup and clang++ with my own. Except still tons of warnings, no break. I could (despite slowness, mostly because tons of useless opengl context switches I think, worked a bit on that too, but patchet is not ready yet) run it, works fine.

Oh yeah, also. I used my system's boost library. That should be default on linux, imo, but it's not my project so... but still. Anyway, boost's version is 1.67 here.

nabaco commented 4 years ago

Hi @bmorel, I have looked at your patch and seems very similar to what I have done in #65, #29, #47. Maybe you can comment there and we can try to progress the existing Pull Requests. Otherwise, if you prefer, we can open a Pull Request with your patch. Feel free to jump to the Gitter channel to discuss this in more detail :smile:

ghost commented 4 years ago

Le Mon, 13 Apr 2020 01:13:38 -0700, Nachum Barcohen notifications@github.com a écrit :

Well, using a webbrowser to browse code is really something I hate (especially on websites using low contrasts, it forces me to move the browser to my main screen). Now that I have replyed to this, at least I can use my mail client for the most basic actions at least... anyway.

About the pull requests you mentioned, it seems 2 are closed and 1 merged, but I still did a quick read there. I noticed 2 things: you tend to not always keep original indentation (I tried my best to avoid it, but I might have done it too ;)) and you use NULL, to compare pointers, which will trigger warnings about implicit cast from pointers to integers. For info, NULL is likely to be implemented like: #define NULL 0 in most C implementations. You should use nullptr instead (since the code uses C++11 now, right?), or just use nothing (which is something used in many C and C++ code bases).

I have not seen anything else to say.

About my patch, you can do the bureaucracy and create a branch, pull code, push code, press several website buttons if you like it. As I said, I don't really care about how this code will end, especially considering that all those stuff on float casts should in practice been fixed "upstream" of the calls, by tweaking the structs or calling gl functions that use doubles. From what I've seen, there is a lot of work to do on that gl stuff anyway, so... I'd say I don't think discussing a trivial patch for weeks is the right thing to do in current state. Either accept or reject, with explanations of why if it is worth it. This way, people can spend time working on code. My opinion only, of course. This is not my project after all, and I don't know if I'll contribute a lot.

nabaco commented 4 years ago

Looking at the last build, it seems that we removed all the C++11 deprecated warnings. Closing the issue.