tdlib / td

Cross-platform library for building Telegram clients
https://core.telegram.org/tdlib
Boost Software License 1.0
7.11k stars 1.44k forks source link

Remove unnecessary data stores #2689

Closed micl2e2 closed 10 months ago

micl2e2 commented 11 months ago

This patch removes some unnecessary data stores in td/telegram/MessagesManager.cpp

levlam commented 11 months ago

You misunderstood the second warning of your static analyzer or compiler, because separator variable is clearly used after the first two assignments and their must not be deleted:

    separator = '&';
  }
  if (!for_group) {
    sb << separator << "single";

The third separator = '&'; assignment is definitely redundant, but removing it just adds a timebomb to the code, which is very likely to transform into a bug when another parameter is added to the link.

The same is true for the old_pts assignment. The variable isn't used after the assignment now, but such code can be added any time in the future and the code must use the updated value of old_pts.

Which static analyzer or compiler produced the warnings?

micl2e2 commented 11 months ago

Thanks for the detailed clarification. I should have kept the 1st and 2nd lines for separator. I am using clang-tidy. For the other two changes, I personally think it's necessary, since I am one of the numerous downstream of tdlib, trully want it to be as perfect as possible. But the final decision would be yours.

levlam commented 11 months ago

Most of default-provided by clang-tidy checks are just garbage, processing which with --fix can easily lead to a code that doesn't compile anymore. Only a few carefully chosen default-provided checks can be useful. clang-tidy can be used to create project-specific checks, but for general analyzis I would suggest to use clang --analyze, MSVC compiler with /analyze, or a commercial software instead.

levlam commented 11 months ago

These particular warnings are reasonable, but it would be wrong to remove the assignments, because the resulting code is extremely error-prone. Instead, the warnings can be silenced by (void)separator usage of the variables before the end of their lifetime.

micl2e2 commented 11 months ago

Just making those warnings silent is never my goal, I believe it is not for any analyzer. What I am thinking about is potentially saving one or more instructions even during debug build, small but good for the final artifacts. If some value assignments are really crucial for the context, yet keeping it would be unnecessary, I would suggest that adding a plain text comment, or just commenting those lines?

levlam commented 11 months ago

What I am thinking about is potentially saving one or more instructions even during debug build

Saving instructions for debug builds is useless and harmful, and in release builds compilers can remove unneeded code themselves. And even they are unable to remove these particular assignments, their impact on performance and code size is 0.000%.

or just commenting those lines?

Keeping them commented will change nothing. Whenever, the code is updated, there is too big chance that now required assignments are still commented.

levlam commented 11 months ago

The purpose of static analyzers and compiler warnings is to improve code and reduce the number of bugs. The proposed changes doesn't help with this and can lead to the opposite.

levlam commented 10 months ago

The mentioned variables are used after the assignments in the latest TDLib version.

micl2e2 commented 10 months ago

The mentioned variables are used after the assignments in the latest TDLib version.

Thanks for your consideration! Then this patch will be centainly unnecessary.