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

fix: compilation on clang-19 #1195

Merged
merged 7 commits into from
Jul 9, 2024
Merged

Conversation

ruslan-ilesik
Copy link
Contributor

@ruslan-ilesik ruslan-ilesik commented Jul 8, 2024

Code change checklist

  • I have ensured that all methods and functions are fully documented using doxygen style comments.
  • My code follows the coding style guide.
  • I tested that my change works before raising the PR.
  • I have ensured that I did not break any existing API calls.
  • I have not built my pull request using AI, a static analysis tool or similar without any human oversight.

Unable to test voice because of broken unit test for voice send.

Replaced std::basic_string with std::vector

Fix for issue 1129

Copy link

netlify bot commented Jul 8, 2024

Deploy Preview for dpp-dev ready!

Name Link
🔨 Latest commit 276714e
🔍 Latest deploy log https://app.netlify.com/sites/dpp-dev/deploys/668d131afdc85500083d8cb4
😎 Deploy Preview https://deploy-preview-1195--dpp-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added documentation Improvements or additions to documentation code Improvements or additions to code. labels Jul 8, 2024
@braindigitalis
Copy link
Contributor

this compiles but have you checked that it works? seems like quite a big change.

@ruslan-ilesik
Copy link
Contributor Author

this compiles but have you checked that it works? seems like quite a big change.

No, I did not yet, I will try to run one of our examples bots tomorrow on this version of build

@ruslan-ilesik
Copy link
Contributor Author

Okay, just checked it using
https://dpp.dev/stream-mp3-discord-bot.html
Works fine.
Also ran code using valgrind, and played song multiple times

==19411== Process terminating with default action of signal 2 (SIGINT)
==19411== at 0x67D7117: __futex_abstimed_wait_common64 (futex-internal.c:57)
==19411== by 0x67D7117: __futex_abstimed_wait_common (futex-internal.c:87)
==19411== by 0x67D7117: __futex_abstimed_wait_cancelable64 (futex-internal.c:139)
==19411== by 0x67D9A40: __pthread_cond_wait_common (pthread_cond_wait.c:503)
==19411== by 0x67D9A40: pthread_cond_wait@@GLIBC_2.3.2 (pthread_cond_wait.c:627)
==19411== by 0x655390E: std::__1::condition_variable::wait(std::__1::unique_lockstd::__1::mutex&) (in /usr/lib/llvm-19/lib/libc++.so.1.0)
==19411== by 0x5AEFC70: dpp::cluster::start(bool)::$_1::operator()() const (in /usr/local/lib/libdpp.so.10.0.31)
==19411== by 0x5AEFB8E: dpp::cluster::start(bool) (in /usr/local/lib/libdpp.so.10.0.31)
==19411== by 0x112D6D: main (in /home/ilesik/test/a.out)
==19411==
==19411== HEAP SUMMARY:
==19411== in use at exit: 18,821,403 bytes in 9,732 blocks
==19411== total heap usage: 72,243 allocs, 62,511 frees, 98,628,767 bytes allocated
==19411==
==19411== LEAK SUMMARY:
==19411== definitely lost: 0 bytes in 0 blocks
==19411== indirectly lost: 0 bytes in 0 blocks
==19411== possibly lost: 8,064 bytes in 19 blocks
==19411== still reachable: 18,813,339 bytes in 9,713 blocks
==19411== suppressed: 0 bytes in 0 blocks
==19411== Rerun with --leak-check=full to see details of leaked memory
==19411==
==19411== For lists of detected and suppressed errors, rerun with: -s
==19411== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

src/dpp/discordvoiceclient.cpp Show resolved Hide resolved
src/dpp/discordvoiceclient.cpp Show resolved Hide resolved
src/dpp/discordvoiceclient.cpp Outdated Show resolved Hide resolved
src/dpp/dispatcher.cpp Show resolved Hide resolved
Copy link
Contributor

@braindigitalis braindigitalis left a comment

Choose a reason for hiding this comment

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

not sure this is right please check it works

@ruslan-ilesik
Copy link
Contributor Author

not sure this is right please check it works

Also, additionally I ran a voice record bot we have, was working just fine (just to double check)

@Jaskowicz1 Jaskowicz1 changed the title #Fix: compilation on clang-19 fix: compilation on clang-19 Jul 9, 2024
@Jaskowicz1 Jaskowicz1 linked an issue Jul 9, 2024 that may be closed by this pull request
Copy link
Contributor

@Jaskowicz1 Jaskowicz1 left a comment

Choose a reason for hiding this comment

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

Looks a-okay to me!

@braindigitalis
Copy link
Contributor

can you please fix the codacy issue by moving the [[maybe_unused]] before the const in the if()

@ruslan-ilesik
Copy link
Contributor Author

can you please fix the codacy issue by moving the [[maybe_unused]] before the const in the if()

How can i do it? This variable was there before my changes, Should i just remove it?

@braindigitalis braindigitalis merged commit 5bf88c2 into brainboxdotcc:dev Jul 9, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Improvements or additions to code. documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

std::basic_string<uint8_t> is broken for LLVM-19
3 participants