[TRIVIAL] fix clang unused-but-set-variable warning #4677
Conversation
(clang 15)
/workspaces/rippled/src/ripple/app/rdb/backend/impl/PostgresDatabase.cpp:178:14: warning: variable 'expNumResults' set but not used [-Wunused-but-set-variable]
uint32_t expNumResults = 1;
StefanVK
changed the title
| @@ -189,8 +187,6 @@ loadLedgerInfos( | |||
| auto minAndMax = | |||
| std::get_if<std::pair<uint32_t, uint32_t>>(&whichLedger)) | |||
| { | |||
| expNumResults = minAndMax->second - minAndMax->first; | |||
There was a problem hiding this comment.
Is it useful to assert the validity of the output of the Postgres database?
expNumResults could be set to an initial absurd value (say -1), later we can do:
if(expNumResults != -1)
assert(expNumResults == res.ntuples());
Similar to the validation at
Is this useful?
|
Hi @StefanVK - could you address the comment above? |
|
@ckeshava and @intelliot I myself would not feel comfortable adding that assert since it's not obvious to me that it's a failure to call this function with a range that exceeds the ledger range in the database. Returning whichever subset of LedgerInfos we have in the database seems like reasonable behavior to me. It's not clear to me that calling the function with a range that is not completely in our database is bug in the calling code and we can and do handle the case gracefully in loadLedgerInfos. If you can argue that it is indeed always a bug to call loadLedgeInfos with a ledgerRange or seq that is not or not fully contained in our database, go ahead and add an assert. I'm not saying it's wrong to add the assert, just that I don't know enough about the function to feel comfortable adding it. |
|
Hello,
I have written the suggested changes at the tip of this branch @scottschurr I'm not familiar with the databases used in |
|
@ckeshava, I get the feeling that your questions are expanding far beyond the boundaries of the initial change request. All of the suggested changes are inside of a conditional macro block. Any questions regarding Postgres should be addressed to @mtrippled, who developed reporting mode. However, fundamentally, this pull request is about a C++ variable that ...
It is possible that the lack of reads of the variable points out a bug in the code. Only @mtrippled is qualified to answer that question. But, lacking that bug, removing the local variable @mtrippled, could you look at the code in question and decide whether you want |
|
@ckeshava What's the answer to your 3rd question? |
|
@mtrippled I don't know the answer myself, I wanted to verify if that situation is possible. If we don't have all the relevant data in the local database, does |
|
@ckeshava I don't have the answer either. But as Scott alluded, before killing the process, make sure of a couple of things first:
Generally, criteria 2 is only met if the alternative is incorrect data being persisted or returned to the user. |
|
okay, I'll dig into it |
|
As it stands, I agree with the changes and it is okay to merge it. |
With clang 15, an unused-but-set-variable warning was emitted:
PostgresDatabase.cpp:178:14: warning: variable 'expNumResults' set but
not used [-Wunused-but-set-variable]
uint32_t expNumResults = 1;
High Level Overview of Change
Fix clang warning about unused but set variable: removed variable.
Context of Change
(clang 15)
/workspaces/rippled/src/ripple/app/rdb/backend/impl/PostgresDatabase.cpp:178:14: warning: variable 'expNumResults' set but not used [-Wunused-but-set-variable]
uint32_t expNumResults = 1;
Type of Change