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

Alvin/minor fixes and improvements #389

Merged

Conversation

alvin-777
Copy link
Contributor

@alvin-777 alvin-777 commented Nov 17, 2020

  • TDD

New/modified code must be backed down with unit test - preferably TDD style development)

  • Documentation

All new/modified functionality should be backed up with API documentation (API.markdown or README.markdown)

Cross-Platform Testing

  • Travis-CI (Linux, OSX) + AppVeyor-CI (Windows)\
  • Optional: Local/VM testing: Windows
  • Optional: Local/VM testing: OSX
  • Optional: Local/VM testing: Linux

Testing Advice

mkdir build; cd build; cmake -DADD_G3LOG_UNIT_TEST=ON ..

Run Test Alternatives:

  • Cross-Platform: ctest
  • or ctest -V for verbose output
  • Linux: make test

Alvin Yu added 2 commits November 10, 2020 11:40
- MinGW g++ doesn't recoganize `-rdynamic`
@@ -134,7 +134,7 @@ target_compile_options(${G3LOG_LIBRARY} PRIVATE
# add GCC specific stuff
target_compile_options(${G3LOG_LIBRARY} PRIVATE
# clang/GCC warnings
$<$<CXX_COMPILER_ID:GNU>:-rdynamic>
$<$<AND:$<CXX_COMPILER_ID:GNU>,$<NOT:$<BOOL:${MINGW}>>>:-rdynamic>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the only line actually modified.

@alvin-777
Copy link
Contributor Author

alvin-777 commented Nov 17, 2020

First commit: fixed a bunch of typos
Second commit: aligned up some indentations and fixed build for MinGW on Windows (here)

Note: even though MinGW builds, it fails the sink_test - specifically, it keeps adding messages to the queue, but the sink threads get starved (or maybe stuck?) and can never get any more messages from some point. Eventually, the test runs out of the memory and Windows crashes (the computer I used has 32GB memory, got filled up within 90 seconds).

If I increase the intervals between logs from 1~10 microseconds to 1~10 milliseconds, then it will pass the test immediately. (this line)

On Ubuntu, it was no problem either.

I don't know if that's a problem with this library or MinGW GCC, still investigating it... But I think this pr is safe to merge. :)

@KjellKod
Copy link
Owner

Good find. AFAIK microseconds handling with $MS might be poor. It probably depends on your setup as well.

@KjellKod KjellKod merged commit c0ae589 into KjellKod:master Nov 18, 2020
@alvin-777 alvin-777 deleted the alvin/minor-fixes-and-improvements branch November 25, 2020 03:26
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