[PATCH v1 11/13] Remove TargetInfo global variable
Chris Johns
chrisj at rtems.org
Mon Aug 2 03:21:17 UTC 2021
On 2/8/21 9:23 am, Ryan Long wrote:
> - Remove TargetInfo from app_common
> - Created the targetInfo_m member variable in CoverageReaderBase,
> TraceWriterBase, and ObjdumpProcessor
> - Made functions to set the value of targetInfo_m
> ---
> tester/covoar/CoverageReaderBase.cc | 5 +++++
> tester/covoar/CoverageReaderBase.h | 12 ++++++++++++
> tester/covoar/CoverageReaderQEMU.cc | 4 ++--
> tester/covoar/ObjdumpProcessor.cc | 29 ++++++++++++++++++-----------
> tester/covoar/ObjdumpProcessor.h | 20 +++++++++++++++++++-
> tester/covoar/TraceConverter.cc | 6 ++++--
> tester/covoar/TraceWriterBase.cc | 5 +++++
> tester/covoar/TraceWriterBase.h | 12 ++++++++++++
> tester/covoar/TraceWriterQEMU.cc | 4 ++--
> tester/covoar/app_common.cc | 1 -
> tester/covoar/app_common.h | 1 -
> tester/covoar/covmerge.cc | 7 ++++++-
> tester/covoar/covoar.cc | 8 ++++++--
> 13 files changed, 91 insertions(+), 23 deletions(-)
>
> diff --git a/tester/covoar/CoverageReaderBase.cc b/tester/covoar/CoverageReaderBase.cc
> index e226964..06732a0 100644
> --- a/tester/covoar/CoverageReaderBase.cc
> +++ b/tester/covoar/CoverageReaderBase.cc
> @@ -21,4 +21,9 @@ namespace Coverage {
> {
> return branchInfoAvailable_m;
> }
> +
> + void CoverageReaderBase::setTargetInfo( Target::TargetBase* targetInfo )
> + {
> + targetInfo_m = targetInfo;
> + }
> }
> diff --git a/tester/covoar/CoverageReaderBase.h b/tester/covoar/CoverageReaderBase.h
> index ba909e6..6c8cadf 100644
> --- a/tester/covoar/CoverageReaderBase.h
> +++ b/tester/covoar/CoverageReaderBase.h
> @@ -49,9 +49,21 @@ namespace Coverage {
> bool getBranchInfoAvailable() const;
>
> /*!
> + * This method sets the targetInfo_m variable
> + *
> + * @param[in] targetInfo the targetInfo to use
> + */
> + void setTargetInfo( Target::TargetBase* targetInfo );
> +
> + /*!
> * This member variable tells whether the branch info is available.
> */
> bool branchInfoAvailable_m = false;
> +
> + /*!
> + * This member variable points to the target's info
> + */
> + Target::TargetBase* targetInfo_m = nullptr;
> };
>
> }
> diff --git a/tester/covoar/CoverageReaderQEMU.cc b/tester/covoar/CoverageReaderQEMU.cc
> index 802d862..a3d9d02 100644
> --- a/tester/covoar/CoverageReaderQEMU.cc
> +++ b/tester/covoar/CoverageReaderQEMU.cc
> @@ -51,8 +51,8 @@ namespace Coverage {
> uint8_t notTaken;
> uint8_t branchInfo;
>
> - taken = TargetInfo->qemuTakenBit();
> - notTaken = TargetInfo->qemuNotTakenBit();
> + taken = targetInfo_m->qemuTakenBit();
> + notTaken = targetInfo_m->qemuNotTakenBit();
> branchInfo = taken | notTaken;
>
> //
> diff --git a/tester/covoar/ObjdumpProcessor.cc b/tester/covoar/ObjdumpProcessor.cc
> index f590ece..5e1fb13 100644
> --- a/tester/covoar/ObjdumpProcessor.cc
> +++ b/tester/covoar/ObjdumpProcessor.cc
> @@ -124,8 +124,10 @@ namespace Coverage {
> }
>
> ObjdumpProcessor::ObjdumpProcessor(
> - DesiredSymbols& symbolsToAnalyze
> - ): symbolsToAnalyze_m( symbolsToAnalyze )
> + DesiredSymbols& symbolsToAnalyze,
> + Target::TargetBase* targetInfo
> + ): symbolsToAnalyze_m( symbolsToAnalyze ),
> + targetInfo_m( targetInfo )
> {
> }
>
> @@ -191,7 +193,7 @@ namespace Coverage {
> const char *instruction
> )
> {
> - if ( !TargetInfo ) {
> + if ( !targetInfo_m ) {
> fprintf(
> stderr,
> "ERROR: ObjdumpProcessor::IsBranch - unknown architecture\n"
> @@ -200,14 +202,14 @@ namespace Coverage {
> return false;
> }
>
> - return TargetInfo->isBranch( instruction );
> + return targetInfo_m->isBranch( instruction );
> }
>
> bool ObjdumpProcessor::isBranchLine(
> const char* const line
> )
> {
> - if ( !TargetInfo ) {
> + if ( !targetInfo_m ) {
> fprintf(
> stderr,
> "ERROR: ObjdumpProcessor::isBranchLine - unknown architecture\n"
> @@ -216,7 +218,7 @@ namespace Coverage {
> return false;
> }
>
> - return TargetInfo->isBranchLine( line );
> + return targetInfo_m->isBranchLine( line );
> }
>
> bool ObjdumpProcessor::isNop(
> @@ -224,7 +226,7 @@ namespace Coverage {
> int& size
> )
> {
> - if ( !TargetInfo ){
> + if ( !targetInfo_m ){
> fprintf(
> stderr,
> "ERROR: ObjdumpProcessor::isNop - unknown architecture\n"
> @@ -233,7 +235,7 @@ namespace Coverage {
> return false;
> }
>
> - return TargetInfo->isNopLine( line, size );
> + return targetInfo_m->isNopLine( line, size );
> }
>
> void ObjdumpProcessor::getFile(
> @@ -243,12 +245,12 @@ namespace Coverage {
> )
> {
> rld::process::status status;
> - rld::process::arg_container args = { TargetInfo->getObjdump(),
> + rld::process::arg_container args = { targetInfo_m->getObjdump(),
> "-Cda", "--section=.text", "--source",
> fileName };
> try
> {
> - status = rld::process::execute( TargetInfo->getObjdump(),
> + status = rld::process::execute( targetInfo_m->getObjdump(),
> args, objdumpFile.name(), err.name() );
> if ( (status.type != rld::process::status::normal)
> || (status.code != 0) ) {
> @@ -256,7 +258,7 @@ namespace Coverage {
> }
> } catch( rld::error& err )
> {
> - std::cout << "Error while running " << TargetInfo->getObjdump()
> + std::cout << "Error while running " << targetInfo_m->getObjdump()
> << " on " << fileName << std::endl;
> std::cout << err.what << " in " << err.where << std::endl;
> return;
> @@ -497,4 +499,9 @@ namespace Coverage {
> }
> }
> }
> +
> + void ObjdumpProcessor::setTargetInfo( Target::TargetBase* targetInfo )
> + {
> + targetInfo_m = targetInfo;
Shared or unique?
> + }
> }
> diff --git a/tester/covoar/ObjdumpProcessor.h b/tester/covoar/ObjdumpProcessor.h
> index c7fc8bb..499af72 100644
> --- a/tester/covoar/ObjdumpProcessor.h
> +++ b/tester/covoar/ObjdumpProcessor.h
> @@ -91,7 +91,8 @@ namespace Coverage {
> * This method constructs an ObjdumpProcessor instance.
> */
> ObjdumpProcessor(
> - DesiredSymbols& symbolsToAnalyze
> + DesiredSymbols& symbolsToAnalyze,
> + Target::TargetBase* targetInfo
std::unique_ptr<> ? Making the change would expose where it is being shared?
A std::shared_ptr<> is better than a naked pointer.
> );
>
> /*!
> @@ -176,9 +177,26 @@ namespace Coverage {
> );
>
> /*!
> + * This method sets the targetInfo_m variable.
> + *
> + * @param[in] targetInfo the pointer to set targetInfo_m to
> + */
> + void setTargetInfo( Target::TargetBase* targetInfo );
> +
> + /*!
> + * This member variable is a buffer for input
> + */
> + char* inputBuffer_m;
> +
> + /*!
> * This member variable contains the symbols to be analyzed
> */
> DesiredSymbols& symbolsToAnalyze_m;
> +
> + /*!
> + * This member variable points to the target's info
> + */
> + Target::TargetBase* targetInfo_m = nullptr;
> };
> }
> #endif
> diff --git a/tester/covoar/TraceConverter.cc b/tester/covoar/TraceConverter.cc
> index 67edd11..7dcaa63 100644
> --- a/tester/covoar/TraceConverter.cc
> +++ b/tester/covoar/TraceConverter.cc
> @@ -92,7 +92,7 @@ int main(
> Coverage::DesiredSymbols symbolsToAnalyze;
> bool verbose = false;
> std::string dynamicLibrary;
> - Coverage::ObjdumpProcessor objdumpProcessor( symbolsToAnalyze );
> + Target::TargetBase* targetInfo;
Again here.
>
> setup_signals();
>
> @@ -130,7 +130,9 @@ int main(
> }
>
> // Create toolnames.
> - TargetInfo = Target::TargetFactory( cpuname );
> + targetInfo = Target::TargetFactory( cpuname );
Return a std ptr of some type?
> +
> + Coverage::ObjdumpProcessor objdumpProcessor( symbolsToAnalyze, targetInfo );
>
> if ( !dynamicLibrary.empty() )
> executableInfo = new Coverage::ExecutableInfo(
> diff --git a/tester/covoar/TraceWriterBase.cc b/tester/covoar/TraceWriterBase.cc
> index 2fa16dc..b86dc97 100644
> --- a/tester/covoar/TraceWriterBase.cc
> +++ b/tester/covoar/TraceWriterBase.cc
> @@ -17,4 +17,9 @@ namespace Trace {
> {
> }
>
> + void TraceWriterBase::setTargetInfo( Target::TargetBase* targetInfo )
> + {
> + targetInfo_m = targetInfo;
> + }
> +
> }
> diff --git a/tester/covoar/TraceWriterBase.h b/tester/covoar/TraceWriterBase.h
> index 070dcca..f29dbcf 100644
> --- a/tester/covoar/TraceWriterBase.h
> +++ b/tester/covoar/TraceWriterBase.h
> @@ -45,6 +45,18 @@ namespace Trace {
> Trace::TraceReaderBase *log,
> bool verbose
> ) = 0;
> +
> + /*!
> + * This method sets the targetInfo_m variable
> + *
> + * @param[in] targetInfo the targetInfo to use
> + */
> + void setTargetInfo( Target::TargetBase* targetInfo );
Why not just directly set it and use it?
This is now 11 years old ...
https://www.drdobbs.com/cpp/how-non-member-functions-improve-encapsu/184401197
The key part is not just using non-member function but not hidding everything in
classes. All you do is duplicate the interface of the private member.
Chris
More information about the devel
mailing list