# A deep look into YTsaurus\. Availability, reliability, open source We have checked YTsaurus with the PVS\-Studio analyzer — let's see the results of this check and the errors found\. It's been over half a year since YTsaurus, a powerful BigData system, became open source\. It has been developed to assist companies in expanding their IT infrastructure and tackling business initiatives\. These days, YTsaurus is a frequent topic for discussion\. According to GitHub statistics, the project is still growing in popularity\. That is why we are here to inspect it\. ![1077_YTsaurus/image1.png](https://import.viva64.com/docx/blog/1077_YTsaurus/image1.png) ## Meet YTsaurus In fact, YTsaurus is frequently and extensively discussed at various conferences which our team attends\. For example, we recently attended the HighLoad\+\+ and TeamLead Conf conferences, and YTsaurus\-related topics were everywhere\. Considering the popularity of the project and my interests in related fields, I started exploring it\. YTsaurus, being an open\-source solution available to every user, happens to have a complete [documentation](https://ytsaurus.tech/docs/en/), and I turned to it\. The home page has both a brief description of the project and a full\-fledged Overview section\. It explains the YTsaurus purpose, describes key features of the platform and the structure of the YTsaurus system\. So, if you're curious, read the section in full — it's going to be even more interesting\. Well, I will not say anything new about the project, except for the results of the code check, of course\. By the way, the project source code is available [here](https://github.com/ytsaurus/ytsaurus)\. Even before I finished checking the project, GitHub was stuffed with new commits\. This is a clear indication that the project is under active development\. The commit I cloned the project on was [55cd85f](https://github.com/ytsaurus/ytsaurus/tree/55cd85f892a252c76137ed4b6f22fd6e0982d9e3)\. This article is not intended to devalue the labor of the YTsaurus developers\. The aim is to popularize static code analyzers, which can be of great assistance even for high\-quality and successful projects\. ## Difficulties we faced We didn't, actually\. "Why have such a catchy headline?", you may ask\. I would like to express my special thanks to the developers of the project\! What detailed instructions YTsaurus has\! And how simple YTsaurus is to deploy\. I do think that even children would be able to deploy YTsaurus if they wanted to\. Well, let's go straight to what we are all looking forward to — code analysis\. ## In the beginning, there was nothing\.\.\. \.\.\.but a bunch of different warnings\. If the project is large enough, the analyzer may find many suspicious code fragments\. This may spoil the first impression of the tool\. A scared user will immediately uninstall the analyzer from their machine\. How can we avoid it? 1. It is necessary to [fine\-tune](https://pvs-studio.com/en/blog/posts/0743/) the analyzer before starting the analysis\. For example, enable the most relevant diagnostic groups and disable the less relevant ones\. There is no point in enabling MISRA/AUTOSAR diagnostic groups if your code is not required to comply with these standards\. 1. After you start the analyzer for the first time, you can display the [most interesting warnings](https://pvs-studio.com/en/docs/manual/6532/)\. This will help you start working with the report much faster and easier\. 1. All these warnings must be handled in a certain way to get all the benefits of regular code analysis\. There are several solutions\. Our team provides a mechanism for [suppressing](https://pvs-studio.com/en/docs/manual/0032/) warnings with suppress files\. Once all warnings have been suppressed, the next time the analysis is run, the report will have no warnings\. 1. Developers do not need to fully analyze the entire project on their machines, as there are usually hundreds of files being changed\. The [incremental analysis mode](https://pvs-studio.com/en/docs/manual/0024/) is a great way to analyze only the files that have been changed\. In this mode, the analyzer issues warnings only for newly written code, and they are few\. If the warning is false, it can be [suppressed directly](https://pvs-studio.com/en/docs/manual/0017/)\. 1. Full project analysis can be integrated into CI during nightly builds\. If you employ the pull/merge request model in project development, you can also [integrate](https://pvs-studio.com/en/docs/manual/0055/) the analysis into your development process\. 1. Sometimes it's worth revisiting old warnings, pulling them out of suppress files, and fixing the code\. When checking projects from time to time, we usually don't go through all of these steps\. However, we do perform some sort of configuration before starting the analysis: we exclude third\-party libraries, enable only General Analysis and micro\-optimizations diagnostic groups\. Sometimes even the finely tuned analyzer may issue a lot of warnings, but it didn't take me long to analyze the errors found in YTsaurus\. While reviewing and examining the analysis results, I found many curious and not\-so\-curious code snippets that I want to share with you\. ## Analysis results **Fragment N1** ```cpp TAsyncSignalsHandler *SIGNALS_HANDLER = nullptr; void SetAsyncSignalHandler(int signum, TAutoPtr handler) { static TAdaptiveLock lock; if (Y_UNLIKELY(SIGNALS_HANDLER == nullptr)) // N1 { TGuard dnd(lock); if (SIGNALS_HANDLER == nullptr) // N2 { // NEVERS GETS DESTROYED SIGNALS_HANDLER = new TAsyncSignalsHandler(); // N3 } } SIGNALS_HANDLER->Install(signum, handler); // N4 } ``` The analyzer warning: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'SIGNALS\_HANDLER \=\= nullptr' is always true\. async\_signals\_handler\.cpp:200 Although the analyzer issued the [V547](https://pvs-studio.com/en/docs/warnings/v547/) warning \(it didn't get the custom locking mechanism quite right\), I detected a curious example of the [Double\-checked locking](https://en.wikipedia.org/wiki/Double-checked_locking) pattern\. What can go wrong? Firstly, we have a small chance that the compiler may cache the value of the _SIGNALS\_HANDLER_ variable in a register and not read the new value again\. At least if unusual locking is used\. The best solution is to declare the variable _volatile_ or use _std::atomic_\. That's not all\. Let's figure out what's wrong in this case\. Let us assume that the _A_ and _B_ threads operate with this code\. The _A_ thread is the first to reach the _N1_ point\. The pointer is null, so the control flow reaches the first _if_ and catches the lock object\. The _A_ thread reaches the _N2_ point\. The pointer is also null, so the control flow reaches the second _if_\. Then, a little magic can happen at the _N3_ point\. The pointer initialization can be roughly represented as follows: ```cpp auto tmp = (TAsyncSignalsHandler *) malloc(sizeof(TAsyncSignalsHandler)); new (tmp) TAsyncSignalsHandler(); SIGNALS_HANDLER = tmp; ``` A clever processor can reorder the instructions, and _SIGNALS\_HANDLER_ will be initialized before _placement new_ is called\. And then the _B_ thread joins the battle — exactly when the _A_ thread hasn't yet called _placement new_\. The control flow enters the _N1_ point, detects the initialized pointer, and moves to the _N4_ point\. In the _B_ thread, the _Install_ member function is called on the object that has not yet started its lifetime\. Undefined behavior\.\.\. How to fix this pattern? We need to make reading and writing to _SIGNALS\_HANDLER_ be executed as atomic operations: ```cpp std::atomic SIGNALS_HANDLER { nullptr }; void SetAsyncSignalHandler(int signum, TAutoPtr handler) { static TAdaptiveLock lock; auto tmp = SIGNALS_HANDLER.load(std::memory_order_acquire); // <= if (Y_UNLIKELY(tmp == nullptr)) { TGuard dnd(lock); tmp = SIGNALS_HANDLER.load(std::memory_order_relaxed); // <= if (tmp == nullptr) { // NEVERS GETS DESTROYED tmp = new TAsyncSignalsHandler(); SIGNALS_HANDLER.store(tmp, std::memory_order_release); // <= } } tmp->Install(signum, handler); } ``` **Fragment N2** ```cpp TSimpleTM& TSimpleTM::Add(EField f, i32 amount) { .... case F_YEAR: { i32 y = amount + (i32)Year; y = ::Min(Max(y, 0), 255 /*max year*/); // YDay may correspond to different MDay if // it's March or greater and the years have different leap status if (Mon > 1) { YDay += (i32)LeapYearAD(RealYear()) - (i32)LeapYearAD(RealYear()); // <= } Year = y; IsLeap = LeapYearAD(RealYear()); return RegenerateFields(); } .... } ``` The analyzer warning: [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions '\(i32\) LeapYearAD\(RealYear\(\)\)' to the left and to the right of the '\-' operator\. datetime\.cpp:154:1 It seems that left and right expressions should not be identical\. But they are, and it results in _YDay \+\= 0_ always occurring\. Sure, we don't know the whole context and therefore can't say with certainty what should be going on in that line and what code should be there\. But in this case, the analyzer is right\. I guess the developer got sidetracked while coding\. **Fragment N3** ```cpp bool operator == (const TCompositePendingJobCount& lhs, const TCompositePendingJobCount& rhs) { if (lhs.DefaultCount != rhs.DefaultCount) { return false; } if (lhs.CountByPoolTree.size() != lhs.CountByPoolTree.size()) // <= { return false; } for (const auto& [tree, lhsCount] : lhs.CountByPoolTree) { .... } return true; } ``` The analyzer warning: [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions 'lhs\.CountByPoolTree\.size\(\)' to the left and to the right of the '\!\=' operator\. structs\.cpp:191 This is another error caused by distraction\. The size of the same container is being compared, which is why the expression is always _false_\. The correct code looks like this: ```cpp if (lhs.CountByPoolTree.size() != rhs.CountByPoolTree.size()){....} ``` Actually, comparisons often run into errors\. Not all of these errors detected by the analyzer are described in this article\. Errors in comparisons are thoroughly described in my colleague's article: "[The evil within the comparison functions](https://pvs-studio.com/en/blog/posts/cpp/0509/)"\. I strongly recommend you read it\. **Fragment N4\.** ```cpp template class TSimpleOperationCommandBase : public virtual TTypedCommandBase { .... public: TSimpleOperationCommandBase() { .... if (!OperationId.IsEmpty() && OperationAlias.operator bool() || OperationId.IsEmpty() && !OperationAlias.operator bool() ) { .... } .... } }; ``` The analyzer warning: [V728](https://pvs-studio.com/en/docs/warnings/v728/) – Message: An excessive check can be simplified\. The '\(A && \!B\) \|\| \(\!A && B\)' expression is equivalent to the 'bool\(A\) \!\= bool\(B\)' expression\. scheduler\_commands\.h:36:1 It may be easier for the programmer who wrote the code to perceive the code above\. Basically, this expression implies the use of the mutually exclusive OR operator for two Boolean operands\. In C and C\+\+, in addition to the bitwise mutually exclusive OR operator, there is also an implicit logical comparison\. This comparison is implemented via the "_\!\=_" operator, and we can use it to rewrite the expression in the following way: ```cpp if (OperationId.IsEmpty() != OperationAlias.operator bool()) ``` The mutually exclusive OR operator returns true only when operands are unequal\. And this is exactly the case where we need the inequality \(_\!\=_\) operator\. As a result, the code becomes more concise\. Here is\.\.\. another example\. **Fragment N5\.** ```cpp int TComparator::CompareKeyBounds(const TKeyBound& lhs, const TKeyBound& rhs, int lowerVsUpper) const { .... // Prefixes coincide. Check if key bounds are indeed at the same point. { auto lhsInclusivenessAsUpper = (lhs.IsUpper && lhs.IsInclusive) || (!lhs.IsUpper && !lhs.IsInclusive); auto rhsInclusivenessAsUpper = (rhs.IsUpper && rhs.IsInclusive) || (!rhs.IsUpper && !rhs.IsInclusive); if (lhsInclusivenessAsUpper != rhsInclusivenessAsUpper) { return lhsInclusivenessAsUpper - rhsInclusivenessAsUpper; } } .... } ``` The analyzer warning: [V728](https://pvs-studio.com/en/docs/warnings/v728/) – Message: An excessive check can be simplified\. The '\(A && B\) \|\| \(\!A && \!B\)' expression is equivalent to the 'bool\(A\) \=\= bool\(B\)' expression\. comparator\.cpp:194:1 This code can be shortened to: ```cpp auto lhsInclusivenessAsUpper = lhs.IsUpper == lhs.IsInclusive; auto rhsInclusivenessAsUpper = rhs.IsUpper == rhs.IsInclusive; ``` Such warnings may be frequent in any project \(YTSaururs has eight of them\)\. It's up to the developers to decide what to do with them\. Some may think that the version before shortening is clearer, others say the shortened code is better\. That's mostly about personal preferences\. However, the PVS\-Studio analyzer can detect such code, which is certainly handy\. If you don't need such warnings in your code, you can always disable them\. **Fragments N6, N7** ```cpp void Reconfigure(TDistributedThrottlerConfigPtr config) override { MemberClient_->Reconfigure(config->MemberClient); DiscoveryClient_->Reconfigure(config->DiscoveryClient); auto oldConfig = Config_.Acquire(); if (oldConfig->LimitUpdatePeriod != config->LimitUpdatePeriod) { UpdateLimitsExecutor_->SetPeriod(config->LimitUpdatePeriod); // <= } if (oldConfig->LeaderUpdatePeriod != config->LeaderUpdatePeriod) { UpdateLeaderExecutor_->SetPeriod(config->LimitUpdatePeriod); // <= } .... Config_.Store(std::move(config)); } ``` The analyzer warning: [V778](https://pvs-studio.com/en/docs/warnings/v778/) Two similar code fragments were found\. Perhaps, this is a typo and 'LeaderUpdatePeriod' variable should be used instead of 'LimitUpdatePeriod'\. distributed\_throttler\.cpp:942, distributed\_throttler\.cpp:939 An error is haunting the code — the error of copy\-paste\. * The developer N1: "Can I copy your condition to write my own?" * The developer N2: "Yeah, just don't do it word for word\." * The developer N1: "OK\!" In this case, it is not clear whether the _LimitUpdatePeriod_ field is used correctly in the second case or whether the _LeaderUpdatePeriod_ field should be used there\. Who knows\.\.\. :\-/ Errors like this are common in any project, here is just another example: ```cpp static void xmlParseCommentComplex(xmlParserCtxtPtr ctxt, xmlChar *buf, size_t len, size_t size) { .... q = CUR_CHAR(ql); .... if (!IS_CHAR(q)) { xmlFatalErrMsgInt(ctxt, XML_ERR_INVALID_CHAR, "xmlParseComment: invalid xmlChar value %d\n", q); .... } .... r = CUR_CHAR(rl); .... if (!IS_CHAR(r)) { xmlFatalErrMsgInt(ctxt, XML_ERR_INVALID_CHAR, "xmlParseComment: invalid xmlChar value %d\n", q); // <= xmlFree (buf); return; } .... } ``` The analyzer warning: [V778](https://pvs-studio.com/en/docs/warnings/v778/) Two similar code fragments were found\. Perhaps, this is a typo and 'r' variable should be used instead of 'q'\. parser\.c:4771:1 Most likely, the _q_ variable is written in the body of the _if\(\!IS\_CHAR\( r \)\)_ condition by mistake, while the _r_ variable should be there instead\. Perhaps, but not for sure\. We can endlessly discuss this "copy\-paste problem" among developers, but the reality is that everyone, in any industry, with every tool, can make mistakes due to fatigue or distractions\. Here, for example, are a hammer and a finger coming face\-to\-face\. You're hammering a nail, and then bang\! Your cursing instantly broadens the vocabulary of everyone around you\. You are upset, hurt and ashamed\. Hahaha, classic\. Be careful and get more rest\! **Fragments N8, N9** ```cpp .... std::vector> BlockWriters_; .... class TPartitionMultiChunkWriter : public TSchemalessMultiChunkWriterBase { .... TPartitionMultiChunkWriter(....) : .... { .... for (int partitionIndex = 0; partitionIndex < partitionCount; ++partitionIndex) { BlockWriters_.emplace_back( new THorizontalBlockWriter(Schema_, BlockReserveSize_) ); .... } .... } } ``` The analyzer warning: [V1023](https://pvs-studio.com/en/docs/warnings/v1023/) A pointer without owner is added to the 'BlockWriters\_' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. schemaless\_chunk\_writer\.cpp:1374 Here is a curious case of a potential memory leak invisible to the eye\. Let's see why it may happen: 1. Everything starts with the creation of an object on the heap via the operator _new_, and then the pointer to that object is passed perfectly to the _emplace\_back_ function\. 1. The vector checks if it can insert the object without reallocating itself\. Let's assume that is not possible \(i\.e\. _capacity\(\) \=\= size\(\)_\)\. The vector then requests a larger memory space from the allocator\. 1. Suppose the allocator cannot comply with the request for some reason\. Then the vector throws an exception while its state remains the same as before the _emplace\_back_ call\. 1. Prepare yourself for a plot twist\. Once _emplace\_back_ causes an exception, the pointer to the object will be lost\. It results in a memory leak\. You can fix it in two ways\. **Way N1**\. Since C\+\+14, if only public constructors of an object are called, you can use the _std::make\_unique_ function: ```cpp BlockWriters_.emplace_back( std::make_unique(Schema_, BlockReserveSize_) ); ``` **Way N2**\. If the _std::make\_unique_ function is not available \(due to the standard version or access to private constructors\), you should wrap the raw pointer into a smart pointer before passing it to the vector: ```cpp std::unique_ptr ptr { new THorizontalBlockWriter(Schema_, BlockReserveSize_) }; BlockWriters_.emplace_back(std::move(ptr)); ``` I also recommend calling _push\_back_ instead of _emplace\_back_\. Since a smart pointer is now being passed to the vector, there will be a call to the move constructor on the object inside the vector anyway\. In the case of _emplace\_back_, the compiler will do extra work to instantiate the variadic function template\. With _push\_back_, the compiler will do less work\. Here is another example of the code with a similar problem\. ```cpp TBatch* GetBatch() { if (IsNewBatchNeeded()) { Batches_.emplace_back(new TBatch()); // <= } return Batches_.back().get(); } ``` The analyzer warning: [V1023](https://pvs-studio.com/en/docs/warnings/v1023/) A pointer without owner is added to the 'Batches\_' container by the 'emplace\_back' method\. A memory leak will occur in case of an exception\. tablet\_request\_batcher\.cpp:244:1 **Fragment N10, N11** ```cpp std::vector BuildThreadSubqueries(....) { .... std::vector subqueries; .... for (auto& stripe : subquery.StripeList->Stripes) { size_t operandIndex = stripe->GetInputStreamIndex(); YT_VERIFY(operandIndex >= 0); .... } .... } ``` The analyzer warning: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'operandIndex \>\= 0' is always true\. Unsigned type value is always \>\= 0\. subquery\.cpp:1061 Before we continue, I suggest looking at the _YT\_VERIFY_ macro: ```cpp #define YT_VERIFY(expr) \ do { \ if (Y_UNLIKELY(!(expr))) { \ YT_ASSERT_TRAP("YT_VERIFY", #expr); \ } \ } while (false) ``` I also add the YT\_ASSERT\_TRAP macro for clarification: ```cpp #define YT_ASSERT_TRAP(trapType, expr) \ ::NYT::NDetail::AssertTrapImpl(TStringBuf(trapType), TStringBuf(expr), __SOURCE_FILE_IMPL__.As(), __LINE__, TStringBuf(__FUNCTION__)); \ Y_UNREACHABLE() ``` The _YT\_VERIFY_ macro is kind of [_assert_](https://en.cppreference.com/w/cpp/error/assert) on steroids, which does not go away even in release builds\. The analyzer reports that the first check — _YT\_VERIFY\(operandIndex \>\= 0\)_ — is always true\. As you can see from the line above, the _operandIndex_ variable is of _size\_t_ type, i\.e\., unsigned\. And as we know, unsigned types do not contain a negative range of values\. I can assume that at some point the _GetInputStreamIndex_ function returned a value of a signed type, and such a check took place\. And here is another similar example where an _unsigned long_ variable is checked for null: ```cpp TChunkIndexBlock BuildChunkIndexBlock( .... ) { .... for(int sectorIndex = 0; sectorIndex < bestFormatDetail->GetSectorCount(); ++sectorIndex) { .... auto paddingSize = THashTableChunkIndexFormatDetail::SectorSize – (buffer - sectorStart) - sizeof(TChecksum); YT_VERIFY(paddingSize >= 0); WriteZeroes(buffer, paddingSize); .... } .... } ``` The analyzer warning: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'paddingSize \>\= 0' is always true\. Unsigned type value is always \>\= 0\. chunk\_index\_builder\.cpp:278:1 Let's take a closer look at the _paddingSize_ declaration: ```cpp auto paddingSize = THashTableChunkIndexFormatDetail::SectorSize – (buffer - sectorStart) - sizeof(TChecksum); ``` The developers may have intended that signed numbers would take precedence, and as a consequence, _paddingSize_ would be of a signed type\. But the _sizeof\(TChecksum\)_ expression has the _size\_t_ type\. Note this line\. **Fragment N12** ```cpp size_t TParallelFileReader::DoReadWithCallback(void* ptr, size_t size, DoReadCallback&& callback) { .... std::optional curBlob; while (curBlob = ReadNextBatch()) { .... } if (....) { return curIdx; } else { size_t prevIdx = curIdx - curBlob->Size(); // <= Y_VERIFY(!BatchTail_); Y_VERIFY(curBlob.has_value()); // <= BatchTail_ = curBlob->SubBlob(size - prevIdx, curBlob->Size()); return size; } } ``` The analyzer warning: [V1007](https://pvs-studio.com/en/docs/warnings/v1007/) The value from the potentially uninitialized optional 'curBlob' is used\. Probably it is a mistake\. parallel\_file\_reader\.cpp:241 To understand this code fragment better, we expand the _Y\_VERIFY_ macro: ```cpp #define Y_VERIFY(expr, ...) \ do{ \ if(Y_UNLIKELY(!(expr))) { \ ::NPrivate::Panic(__SOURCE_FILE_IMPL__, __LINE__, __FUNCTION__, #expr, " " __VA_ARGS__); \ } \ } while (false) ``` Let's get into it\. The code has the _curBlob_ variable of optional type\. Next, the _ReadNextBatch_ function, which initializes _curBlob_, is called in a loop\. The loop exits in two cases: * the loop body contains _break_; * the call to _ReadNextBatch_ returns _std::nullopt_\. Suppose the loop exits for the second reason, and the control flow enters the _else_ branch\. Then, when the initializer of the _prevIdx_ variable is evaluated, the null object is accessed, which leads to undefined behavior\. Look at the code two lines below\. It seems that the developer anticipated that this might happen, so they added this check\. But it was too late, the horse had already bolted\. I would suggest fixing the code as follows, by moving the check above the dereference: ```cpp Y_VERIFY(!BatchTail_); Y_VERIFY(curBlob.has_value()); size_t prevIdx = curIdx - curBlob->Size(); ``` **Fragment N13** ```cpp template class TInteger128 { .... }; using ui128 = TInteger128; using i128 = TInteger128; // specialize std templates namespace std { .... constexpr bool signbit(const ui128 arg) noexcept { Y_UNUSED(arg); return false; } constexpr bool signbit(const i128 arg) noexcept { return GetHigh(arg) & 0x8000000000000000; } constexpr ui128 abs(const ui128 arg) noexcept { return arg; } constexpr i128 abs(const i128 arg) noexcept { return signbit(arg) ? (-arg) : arg; } } ``` The analyzer warning: [V1061](https://pvs-studio.com/en/docs/warnings/v1061/) Extending the 'std' namespace may result in undefined behavior\. int128\.h:378:1 We love C\+\+ and its standards\! Standards help us understand how a particular tool is supposed to work\. The C\+\+ Standards Committee has established the rules for extending the _std_ namespace\. These rules state that the namespace cannot be extended by declaring functions, variables, or classes, as this can lead to undefined behavior\. The above example shows that the developer wanted to add overloads of standard functions for a 128\-bit integer \(_TInteger128_\)\. However, this extending leads to undefined behavior\. The correct place to add such functions is in the same namespace where the class is declared\. Then at each place where a standard function is needed, its unqualified name should be used: ```cpp template void foo(T &arg) { using std::abs; arg = abs(arg); .... } ``` Wrong _std_ namespace extensions are so frequent that we have a ranking, and _std::swap_ and _std::hash_ are at the top of it\. Speaking of them, actually\. **Fragment N14\.** ```cpp namespace std { ///////////////////////////////////////////////////////////////////// template void swap(NYT::TCompactVector& lhs, NYT::TCompactVector& rhs) { lhs.swap(rhs); } ///////////////////////////////////////////////////////////////////// template struct hash> { size_t operator()(const NYT::TCompactVector& container) const { size_t result = 0; for (const auto& element : container) { NYT::HashCombine(result, element); } return result; } }; ///////////////////////////////////////////////////////////////////// } ``` The analyzer warning: [V1061](https://pvs-studio.com/en/docs/warnings/v1061/) Extending the 'std' namespace may result in undefined behavior\. compact\_vector\-inl\.h:999:1 Speaking of the custom _std::swap_ function template, the C\+\+ standard forbids adding new templates to the _std_ namespace\. This function should be moved to the _NYT_ namespace There is nothing wrong with the _std::hash_ class template specialization in this case\. But it can be taken outside the _std_ namespace\. At least, to avoid enticing other developers to continue the tradition of extending _std_: ```cpp template struct std::hash> { .... }; ``` **Fragment N15** ```cpp IUnversionedRowBatchPtr THorizontalSchemalessRangeChunkReader::Read( const TRowBatchReadOptions& options) { TCurrentTraceContextGuard traceGuard(TraceContext_); auto readGuard = AcquireReadGuard(); // <= .... } ``` The analyzer warning: [V1002](https://pvs-studio.com/en/docs/warnings/v1002/) The 'TTimerGuard' class, containing pointers, constructor and destructor, is copied by the automatically generated copy constructor\. schemaless\_chunk\_reader\.cpp:731 In this case, the analyzer has detected the _TTimerGuard_, class template that may work incorrectly\. This class template functions as an RAII wrapper over the timers\. Let's look at its implementation: ```cpp template class TTimerGuard { public: explicit TTimerGuard(TTimer* timer); ~TTimerGuard(); private: TTimer* const Timer_; }; ``` ```cpp template TTimerGuard::TTimerGuard(TTimer* timer) : Timer_(timer) { Timer_->Start(); } template TTimerGuard::~TTimerGuard() { Timer_->Stop(); } ``` As you can see, the constructor implements capturing the timer and starting it, while the destructor is responsible for stopping it\. However, this class template will also have an implicit copy constructor/ operator generated\. This may cause the object to be copied\. And then it will cause the stop function to be called multiple times on the same timer\. This won't necessarily cause an error, but the design of such a class template shouldn't give that any chance\. If this class template should have semantics similar to [_std::lock\_guard_](https://en.cppreference.com/w/cpp/thread/lock_guard), it will be enough to make it uncopiable: ```cpp template class TTimerGuard { public: explicit TTimerGuard(TTimer* timer); TTimerGuard(const TTimerGuard &) = delete; TTimerGuard& operator=(const TTimerGuard &) = delete; ~TTimerGuard(); private: TTimer* const Timer_; }; ``` If objects are required to be movable \(e\.g\., like [_std::unique\_lock_](https://en.cppreference.com/w/cpp/thread/unique_lock)\), the implementation needs to be changed a bit: ```cpp template class TTimerGuard { public: TTimerGuard() = default; explicit TTimerGuard(TTimer* timer) noexcept(noexcept(timer->Start())); TTimerGuard(TTimerGuard &&other) noexcept; TTimerGuard& operator=(TTimerGuard &&other) noexcept; ~TTimerGuard(); private: TTimer* Timer_ = {}; }; template TTimerGuard::TTimerGuard(TTimer *timer) noexcept(noexcept(timer->Start())) : Timer_ { timer } { if (Timer_) Timer_->Start(); } template TTimerGuard::TTimerGuard(TTimerGuard &&other) noexcept : Timer_ { std::exchange(other.Timer_, {}) } { } template TTimerGuard& TTimerGuard::operator=(TTimerGuard &&other) noexcept { Timer_ = std::exchange(other.Timer_, {}); return *this; } template TTimerGuard::~ TTimerGuard() { if (Timer_) Timer_->Stop(); } ``` **Fragment N16** ```cpp TDivisors GetDivisors(const TSchemaColumns& columns, int keyIndex, TConstExpressionPtr expr) { .... if (binaryOp->Opcode == EBinaryOp::Divide) { .... } else if (binaryOp->Opcode == EBinaryOp::RightShift) { TUnversionedValue value = literal->Value; value.Data.Uint64 = 1 << value.Data.Uint64; // <= value.Id = 0; return TDivisors{value}; } .... } ``` The analyzer warning: [V629](https://pvs-studio.com/en/docs/warnings/v629/) Consider inspecting the '1 << value\.Data\.Uint64' expression\. Bit shifting of the 32\-bit value with a subsequent expansion to the 64\-bit type\. range\_inferrer\.cpp:378 Such code is considered potentially dangerous\. The literal _1_ of the _int_ type is shifted to the left by _value\.Data\.Unit64_\. As a result, the value of such an expression may not be defined\. If _value\.Data\.Uint64_ is within the range of \[32 \.\.\. 63\], a sign overflow occurs\. The bitwise shift operator [will not change](https://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators) the type of the resulting expression, which will still be of the _int_ type\. Even though the right operand is already of the right type\. The [proof](https://godbolt.org/z/M4zMssGaz)\. The analyzer indicates such a range because the resulting expression is then being cast to a larger type\. Fixing the problem is quite easy, you just need to cast the 1 literal to the correct type: ```cpp value.Data.Uint64 = (uint64_t) 1 << value.Data.Uint64; ``` You may ask, "What's so dangerous about it? Will it explode or won't start?" The answer is, "It will start\.\.\." Most likely, everything is working fine now and will continue to do so if overflow is avoided\. But it can "explode", actually\. There's no point in guessing how an error will manifest itself when you enter the territory of undefined behavior\. And if you really want to, I suggest you read a great article on this topic: "[Falsehoods programmers believe about undefined behavior](https://pvs-studio.com/en/blog/posts/cpp/1024/)"\. ## Conclusion The developers of the project took safety seriously\. They implemented many additional checks and were careful enough to provide comments that clarify the logic of the code\. The code is all clear and beautiful\. This truly deserves our respect\. It's okay to make mistakes\. A good programmer should know how to write good code and how to avoid writing bad one\. I hope you learned something new or even helpful from this code fragment examination, analyzer warnings, and code faults\. As usual, I suggest [trying](https://pvs-studio.com/en/pvs-studio/try-free/) the PVS\-Studio analyzer\. We have a [free license](https://pvs-studio.com/en/order/open-source-license/) for open\-source projects\. Take care, and have a good day\!